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

[PATCH v7 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---

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)
    v5: - Fix 'reg' not in <addr, len> pairs
    	- Add missing include to devicetree example
    v6-v7: No change

 .../bindings/remoteproc/ingenic,vpu.yaml      | 77 +++++++++++++++++++
 1 file changed, 77 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..c019f9fbe916
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
@@ -0,0 +1,77 @@
+# 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:
+  - |
+    #include <dt-bindings/clock/jz4770-cgu.h>
+
+    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.26.2

[PATCH v7 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>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Notes:
    v3: New patch
    v4: No change
    v5: - Fix return value documentation
    	- Fix typo in documentation
    v6-v7: 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 e12a54e67588..a7f96bc98406 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1949,6 +1949,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, negative errno on failure
+ */
+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
@@ -2171,6 +2198,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 acquired rproc device will
+ * automatically be released on driver detach.
+ *
+ * Returns: new rproc instance, or NULL on failure
+ */
+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 9c07d7958c53..8c9c0dda03c3 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -599,6 +599,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.26.2

[PATCH v7 3/5] remoteproc: Add support for runtime PM

[Paul Cercueil]

Call pm_runtime_get_sync() before the firmware is loaded, and
pm_runtime_put() after the remote processor has been stopped.

Even though the remoteproc device has no PM callbacks, this allows the
parent device's PM callbacks to be properly called.

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

Notes:
    v2-v4: No change
    v5: Move calls to prepare/unprepare to rproc_fw_boot/rproc_shutdown
    v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
    v7: Check return value of pm_runtime_get_sync()

 drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index a7f96bc98406..e33d1ef27981 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -29,6 +29,7 @@
 #include <linux/devcoredump.h>
 #include <linux/rculist.h>
 #include <linux/remoteproc.h>
+#include <linux/pm_runtime.h>
 #include <linux/iommu.h>
 #include <linux/idr.h>
 #include <linux/elf.h>
@@ -1382,6 +1383,12 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	if (ret)
 		return ret;
 
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
+		return ret;
+	}
+
 	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
 
 	/*
@@ -1391,7 +1398,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	ret = rproc_enable_iommu(rproc);
 	if (ret) {
 		dev_err(dev, "can't enable iommu: %d\n", ret);
-		return ret;
+		goto put_pm_runtime;
 	}
 
 	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
@@ -1435,6 +1442,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	rproc->table_ptr = NULL;
 disable_iommu:
 	rproc_disable_iommu(rproc);
+put_pm_runtime:
+	pm_runtime_put(dev);
 	return ret;
 }
 
@@ -1840,6 +1849,8 @@ void rproc_shutdown(struct rproc *rproc)
 
 	rproc_disable_iommu(rproc);
 
+	pm_runtime_put(dev);
+
 	/* Free the copy of the resource table */
 	kfree(rproc->cached_table);
 	rproc->cached_table = NULL;
@@ -2118,6 +2129,9 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 
 	rproc->state = RPROC_OFFLINE;
 
+	pm_runtime_no_callbacks(&rproc->dev);
+	pm_runtime_enable(&rproc->dev);
+
 	return rproc;
 }
 EXPORT_SYMBOL(rproc_alloc);
@@ -2133,6 +2147,7 @@ EXPORT_SYMBOL(rproc_alloc);
  */
 void rproc_free(struct rproc *rproc)
 {
+	pm_runtime_disable(&rproc->dev);
 	put_device(&rproc->dev);
 }
 EXPORT_SYMBOL(rproc_free);
-- 
2.26.2

[PATCH v7 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>
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()
    v5: No change
    v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
    v7: - Remove use of of_match_ptr()
    	- Move Kconfig symbol so that it's in alphabetical order
    	- Add missing doc for private structure field aux_base
    	- Don't check for (len <= 0) in da_to_va()
    	- Add missing \n in dev_info/dev_err messages

 drivers/remoteproc/Kconfig         |   9 +
 drivers/remoteproc/Makefile        |   1 +
 drivers/remoteproc/ingenic_rproc.c | 280 +++++++++++++++++++++++++++++
 3 files changed, 290 insertions(+)
 create mode 100644 drivers/remoteproc/ingenic_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index fbaed079b299..c4d1731295eb 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -23,6 +23,15 @@ config IMX_REMOTEPROC
 
 	  It's safe to say N here.
 
+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.
+
 config MTK_SCP
 	tristate "Mediatek SCP support"
 	depends on ARCH_MEDIATEK
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 0effd3825035..e8b886e511f0 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
new file mode 100644
index 000000000000..189020d77b25
--- /dev/null
+++ b/drivers/remoteproc/ingenic_rproc.c
@@ -0,0 +1,280 @@
+// 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/pm_runtime.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
+ * @aux_base: raw pointer to the AUX interface registers
+ * @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_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, size_t len)
+{
+	struct vpu *vpu = rproc->priv;
+	void __iomem *va = NULL;
+	unsigned int i;
+
+	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 = {
+	.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 void ingenic_rproc_disable_clks(void *data)
+{
+	struct vpu *vpu = data;
+
+	pm_runtime_resume(vpu->dev);
+	pm_runtime_disable(vpu->dev);
+
+	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
+}
+
+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;
+	platform_set_drvdata(pdev, vpu);
+
+	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\n");
+		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\n");
+			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\n");
+		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\n");
+		return ret;
+	}
+
+	disable_irq(vpu->irq);
+
+	/* 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(dev, "Unable to start clocks\n");
+		return ret;
+	}
+
+	pm_runtime_irq_safe(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+	pm_runtime_use_autosuspend(dev);
+
+	ret = devm_add_action_or_reset(dev, ingenic_rproc_disable_clks, vpu);
+	if (ret) {
+		dev_err(dev, "Unable to register action\n");
+		goto out_pm_put;
+	}
+
+	ret = devm_rproc_add(dev, rproc);
+	if (ret) {
+		dev_err(dev, "Failed to register remote processor\n");
+		goto out_pm_put;
+	}
+
+out_pm_put:
+	pm_runtime_put_autosuspend(dev);
+
+	return ret;
+}
+
+static const struct of_device_id ingenic_rproc_of_matches[] = {
+	{ .compatible = "ingenic,jz4770-vpu-rproc", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
+
+static int __maybe_unused ingenic_rproc_suspend(struct device *dev)
+{
+	struct vpu *vpu = dev_get_drvdata(dev);
+
+	clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
+
+	return 0;
+}
+
+static int __maybe_unused ingenic_rproc_resume(struct device *dev)
+{
+	struct vpu *vpu = dev_get_drvdata(dev);
+
+	return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
+}
+
+static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
+	SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, ingenic_rproc_resume, NULL)
+};
+
+static struct platform_driver ingenic_rproc_driver = {
+	.probe = ingenic_rproc_probe,
+	.driver = {
+		.name = "ingenic-vpu",
+#ifdef CONFIG_PM
+		.pm = &ingenic_rproc_pm,
+#endif
+		.of_match_table = 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.26.2

[PATCH v7 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
    v5-v7: No change

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 091ec22c1a23..5d864d785574 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8414,6 +8414,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.26.2

Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver

[Bjorn Andersson]

On Fri 15 May 03:43 PDT 2020, Paul Cercueil wrote:

> 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>
> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Series applied

Thanks,
Bjorn

> ---
> 
> 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()
>     v5: No change
>     v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
>     v7: - Remove use of of_match_ptr()
>     	- Move Kconfig symbol so that it's in alphabetical order
>     	- Add missing doc for private structure field aux_base
>     	- Don't check for (len <= 0) in da_to_va()
>     	- Add missing \n in dev_info/dev_err messages
> 
>  drivers/remoteproc/Kconfig         |   9 +
>  drivers/remoteproc/Makefile        |   1 +
>  drivers/remoteproc/ingenic_rproc.c | 280 +++++++++++++++++++++++++++++
>  3 files changed, 290 insertions(+)
>  create mode 100644 drivers/remoteproc/ingenic_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index fbaed079b299..c4d1731295eb 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -23,6 +23,15 @@ config IMX_REMOTEPROC
>  
>  	  It's safe to say N here.
>  
> +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.
> +
>  config MTK_SCP
>  	tristate "Mediatek SCP support"
>  	depends on ARCH_MEDIATEK
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 0effd3825035..e8b886e511f0 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>  obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
> diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
> new file mode 100644
> index 000000000000..189020d77b25
> --- /dev/null
> +++ b/drivers/remoteproc/ingenic_rproc.c
> @@ -0,0 +1,280 @@
> +// 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/pm_runtime.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
> + * @aux_base: raw pointer to the AUX interface registers
> + * @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_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, size_t len)
> +{
> +	struct vpu *vpu = rproc->priv;
> +	void __iomem *va = NULL;
> +	unsigned int i;
> +
> +	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 = {
> +	.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 void ingenic_rproc_disable_clks(void *data)
> +{
> +	struct vpu *vpu = data;
> +
> +	pm_runtime_resume(vpu->dev);
> +	pm_runtime_disable(vpu->dev);
> +
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
> +}
> +
> +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;
> +	platform_set_drvdata(pdev, vpu);
> +
> +	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\n");
> +		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\n");
> +			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\n");
> +		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\n");
> +		return ret;
> +	}
> +
> +	disable_irq(vpu->irq);
> +
> +	/* 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(dev, "Unable to start clocks\n");
> +		return ret;
> +	}
> +
> +	pm_runtime_irq_safe(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = devm_add_action_or_reset(dev, ingenic_rproc_disable_clks, vpu);
> +	if (ret) {
> +		dev_err(dev, "Unable to register action\n");
> +		goto out_pm_put;
> +	}
> +
> +	ret = devm_rproc_add(dev, rproc);
> +	if (ret) {
> +		dev_err(dev, "Failed to register remote processor\n");
> +		goto out_pm_put;
> +	}
> +
> +out_pm_put:
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id ingenic_rproc_of_matches[] = {
> +	{ .compatible = "ingenic,jz4770-vpu-rproc", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
> +
> +static int __maybe_unused ingenic_rproc_suspend(struct device *dev)
> +{
> +	struct vpu *vpu = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ingenic_rproc_resume(struct device *dev)
> +{
> +	struct vpu *vpu = dev_get_drvdata(dev);
> +
> +	return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
> +}
> +
> +static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
> +	SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, ingenic_rproc_resume, NULL)
> +};
> +
> +static struct platform_driver ingenic_rproc_driver = {
> +	.probe = ingenic_rproc_probe,
> +	.driver = {
> +		.name = "ingenic-vpu",
> +#ifdef CONFIG_PM
> +		.pm = &ingenic_rproc_pm,
> +#endif
> +		.of_match_table = 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.26.2
> 

Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM

[Suman Anna]

Hi Paul,

On 5/15/20 5:43 AM, Paul Cercueil wrote:
> Call pm_runtime_get_sync() before the firmware is loaded, and
> pm_runtime_put() after the remote processor has been stopped.
> 
> Even though the remoteproc device has no PM callbacks, this allows the
> parent device's PM callbacks to be properly called.

I see this patch staged now for 5.8, and the latest -next branch has 
broken the pm-runtime autosuspend feature we have in the OMAP remoteproc 
driver. See commit 5f31b232c674 ("remoteproc/omap: Add support for 
runtime auto-suspend/resume").

What was the original purpose of this patch, because there can be 
differing backends across different SoCs.

regards
Suman

> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>      v2-v4: No change
>      v5: Move calls to prepare/unprepare to rproc_fw_boot/rproc_shutdown
>      v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
>      v7: Check return value of pm_runtime_get_sync()
> 
>   drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index a7f96bc98406..e33d1ef27981 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -29,6 +29,7 @@
>   #include <linux/devcoredump.h>
>   #include <linux/rculist.h>
>   #include <linux/remoteproc.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/iommu.h>
>   #include <linux/idr.h>
>   #include <linux/elf.h>
> @@ -1382,6 +1383,12 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>   	if (ret)
>   		return ret;
>   
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
> +		return ret;
> +	}
> +
>   	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>   
>   	/*
> @@ -1391,7 +1398,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>   	ret = rproc_enable_iommu(rproc);
>   	if (ret) {
>   		dev_err(dev, "can't enable iommu: %d\n", ret);
> -		return ret;
> +		goto put_pm_runtime;
>   	}
>   
>   	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
> @@ -1435,6 +1442,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>   	rproc->table_ptr = NULL;
>   disable_iommu:
>   	rproc_disable_iommu(rproc);
> +put_pm_runtime:
> +	pm_runtime_put(dev);
>   	return ret;
>   }
>   
> @@ -1840,6 +1849,8 @@ void rproc_shutdown(struct rproc *rproc)
>   
>   	rproc_disable_iommu(rproc);
>   
> +	pm_runtime_put(dev);
> +
>   	/* Free the copy of the resource table */
>   	kfree(rproc->cached_table);
>   	rproc->cached_table = NULL;
> @@ -2118,6 +2129,9 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>   
>   	rproc->state = RPROC_OFFLINE;
>   
> +	pm_runtime_no_callbacks(&rproc->dev);
> +	pm_runtime_enable(&rproc->dev);
> +
>   	return rproc;
>   }
>   EXPORT_SYMBOL(rproc_alloc);
> @@ -2133,6 +2147,7 @@ EXPORT_SYMBOL(rproc_alloc);
>    */
>   void rproc_free(struct rproc *rproc)
>   {
> +	pm_runtime_disable(&rproc->dev);
>   	put_device(&rproc->dev);
>   }
>   EXPORT_SYMBOL(rproc_free);
> 

Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM

[Paul Cercueil]

Hi Suman,

Le ven. 22 mai 2020 à 11:47, Suman Anna <s-anna@ti.com> a écrit :
> Hi Paul,
> 
> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>> Call pm_runtime_get_sync() before the firmware is loaded, and
>> pm_runtime_put() after the remote processor has been stopped.
>> 
>> Even though the remoteproc device has no PM callbacks, this allows 
>> the
>> parent device's PM callbacks to be properly called.
> 
> I see this patch staged now for 5.8, and the latest -next branch has 
> broken the pm-runtime autosuspend feature we have in the OMAP 
> remoteproc driver. See commit 5f31b232c674 ("remoteproc/omap: Add 
> support for runtime auto-suspend/resume").
> 
> What was the original purpose of this patch, because there can be 
> differing backends across different SoCs.

Did you try pm_suspend_ignore_children()? It looks like it was made for 
your use-case.

Cheers,
-Paul

> 
> regards
> Suman
> 
>> 
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> ---
>> 
>> Notes:
>>      v2-v4: No change
>>      v5: Move calls to prepare/unprepare to 
>> rproc_fw_boot/rproc_shutdown
>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>> callbacks
>>      v7: Check return value of pm_runtime_get_sync()
>> 
>>   drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/remoteproc/remoteproc_core.c 
>> b/drivers/remoteproc/remoteproc_core.c
>> index a7f96bc98406..e33d1ef27981 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/devcoredump.h>
>>   #include <linux/rculist.h>
>>   #include <linux/remoteproc.h>
>> +#include <linux/pm_runtime.h>
>>   #include <linux/iommu.h>
>>   #include <linux/idr.h>
>>   #include <linux/elf.h>
>> @@ -1382,6 +1383,12 @@ static int rproc_fw_boot(struct rproc *rproc, 
>> const struct firmware *fw)
>>   	if (ret)
>>   		return ret;
>>   \x7f+	ret = pm_runtime_get_sync(dev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>>   	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>>   \x7f  	/*
>> @@ -1391,7 +1398,7 @@ static int rproc_fw_boot(struct rproc *rproc, 
>> const struct firmware *fw)
>>   	ret = rproc_enable_iommu(rproc);
>>   	if (ret) {
>>   		dev_err(dev, "can't enable iommu: %d\n", ret);
>> -		return ret;
>> +		goto put_pm_runtime;
>>   	}
>>   \x7f  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>> @@ -1435,6 +1442,8 @@ static int rproc_fw_boot(struct rproc *rproc, 
>> const struct firmware *fw)
>>   	rproc->table_ptr = NULL;
>>   disable_iommu:
>>   	rproc_disable_iommu(rproc);
>> +put_pm_runtime:
>> +	pm_runtime_put(dev);
>>   	return ret;
>>   }
>>   \x7f@@ -1840,6 +1849,8 @@ void rproc_shutdown(struct rproc *rproc)
>>   \x7f  	rproc_disable_iommu(rproc);
>>   \x7f+	pm_runtime_put(dev);
>> +
>>   	/* Free the copy of the resource table */
>>   	kfree(rproc->cached_table);
>>   	rproc->cached_table = NULL;
>> @@ -2118,6 +2129,9 @@ struct rproc *rproc_alloc(struct device *dev, 
>> const char *name,
>>   \x7f  	rproc->state = RPROC_OFFLINE;
>>   \x7f+	pm_runtime_no_callbacks(&rproc->dev);
>> +	pm_runtime_enable(&rproc->dev);
>> +
>>   	return rproc;
>>   }
>>   EXPORT_SYMBOL(rproc_alloc);
>> @@ -2133,6 +2147,7 @@ EXPORT_SYMBOL(rproc_alloc);
>>    */
>>   void rproc_free(struct rproc *rproc)
>>   {
>> +	pm_runtime_disable(&rproc->dev);
>>   	put_device(&rproc->dev);
>>   }
>>   EXPORT_SYMBOL(rproc_free);
>> 
> 

Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM

[Suman Anna]

Hi Paul,

On 5/22/20 12:11 PM, Paul Cercueil wrote:
> Hi Suman,
> 
> Le ven. 22 mai 2020 à 11:47, Suman Anna <s-anna@ti.com> a écrit :
>> Hi Paul,
>>
>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>> Call pm_runtime_get_sync() before the firmware is loaded, and
>>> pm_runtime_put() after the remote processor has been stopped.
>>>
>>> Even though the remoteproc device has no PM callbacks, this allows the
>>> parent device's PM callbacks to be properly called.
>>
>> I see this patch staged now for 5.8, and the latest -next branch has 
>> broken the pm-runtime autosuspend feature we have in the OMAP 
>> remoteproc driver. See commit 5f31b232c674 ("remoteproc/omap: Add 
>> support for runtime auto-suspend/resume").
>>
>> What was the original purpose of this patch, because there can be 
>> differing backends across different SoCs.
> 
> Did you try pm_suspend_ignore_children()? It looks like it was made for 
> your use-case.

Sorry for the delay in getting back. So, using 
pm_suspend_ignore_children() does fix my current issue.

But I still fail to see the original purpose of this patch in the 
remoteproc core especially given that the core itself does not have any 
callbacks. If the sole intention was to call the parent pdev's 
callbacks, then I feel that state-machine is better managed within that 
particular platform driver itself, as the sequencing/device management 
can vary with different platform drivers.

regards
Suman

> 
> Cheers,
> -Paul
> 
>>
>> regards
>> Suman
>>
>>>
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>> ---
>>>
>>> Notes:
>>>      v2-v4: No change
>>>      v5: Move calls to prepare/unprepare to rproc_fw_boot/rproc_shutdown
>>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>>> callbacks
>>>      v7: Check return value of pm_runtime_get_sync()
>>>
>>>   drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++++-
>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c 
>>> b/drivers/remoteproc/remoteproc_core.c
>>> index a7f96bc98406..e33d1ef27981 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -29,6 +29,7 @@
>>>   #include <linux/devcoredump.h>
>>>   #include <linux/rculist.h>
>>>   #include <linux/remoteproc.h>
>>> +#include <linux/pm_runtime.h>
>>>   #include <linux/iommu.h>
>>>   #include <linux/idr.h>
>>>   #include <linux/elf.h>
>>> @@ -1382,6 +1383,12 @@ static int rproc_fw_boot(struct rproc *rproc, 
>>> const struct firmware *fw)
>>>       if (ret)
>>>           return ret;
>>>   \x7f+    ret = pm_runtime_get_sync(dev);
>>> +    if (ret < 0) {
>>> +        dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>>       dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>>>   \x7f      /*
>>> @@ -1391,7 +1398,7 @@ static int rproc_fw_boot(struct rproc *rproc, 
>>> const struct firmware *fw)
>>>       ret = rproc_enable_iommu(rproc);
>>>       if (ret) {
>>>           dev_err(dev, "can't enable iommu: %d\n", ret);
>>> -        return ret;
>>> +        goto put_pm_runtime;
>>>       }
>>>   \x7f      rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>>> @@ -1435,6 +1442,8 @@ static int rproc_fw_boot(struct rproc *rproc, 
>>> const struct firmware *fw)
>>>       rproc->table_ptr = NULL;
>>>   disable_iommu:
>>>       rproc_disable_iommu(rproc);
>>> +put_pm_runtime:
>>> +    pm_runtime_put(dev);
>>>       return ret;
>>>   }
>>>   \x7f@@ -1840,6 +1849,8 @@ void rproc_shutdown(struct rproc *rproc)
>>>   \x7f      rproc_disable_iommu(rproc);
>>>   \x7f+    pm_runtime_put(dev);
>>> +
>>>       /* Free the copy of the resource table */
>>>       kfree(rproc->cached_table);
>>>       rproc->cached_table = NULL;
>>> @@ -2118,6 +2129,9 @@ struct rproc *rproc_alloc(struct device *dev, 
>>> const char *name,
>>>   \x7f      rproc->state = RPROC_OFFLINE;
>>>   \x7f+    pm_runtime_no_callbacks(&rproc->dev);
>>> +    pm_runtime_enable(&rproc->dev);
>>> +
>>>       return rproc;
>>>   }
>>>   EXPORT_SYMBOL(rproc_alloc);
>>> @@ -2133,6 +2147,7 @@ EXPORT_SYMBOL(rproc_alloc);
>>>    */
>>>   void rproc_free(struct rproc *rproc)
>>>   {
>>> +    pm_runtime_disable(&rproc->dev);
>>>       put_device(&rproc->dev);
>>>   }
>>>   EXPORT_SYMBOL(rproc_free);
>>>
>>
> 
> 

Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM

[Paul Cercueil]

Hi Suman,

>>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>>> Call pm_runtime_get_sync() before the firmware is loaded, and
>>>> pm_runtime_put() after the remote processor has been stopped.
>>>> 
>>>> Even though the remoteproc device has no PM callbacks, this allows 
>>>> the
>>>> parent device's PM callbacks to be properly called.
>>> 
>>> I see this patch staged now for 5.8, and the latest -next branch 
>>> has \x7f\x7fbroken the pm-runtime autosuspend feature we have in the OMAP 
>>> \x7f\x7fremoteproc driver. See commit 5f31b232c674 ("remoteproc/omap: Add 
>>> \x7f\x7fsupport for runtime auto-suspend/resume").
>>> 
>>> What was the original purpose of this patch, because there can be 
>>> \x7f\x7fdiffering backends across different SoCs.
>> 
>> Did you try pm_suspend_ignore_children()? It looks like it was made 
>> for \x7fyour use-case.
> 
> Sorry for the delay in getting back. So, using 
> pm_suspend_ignore_children() does fix my current issue.
> 
> But I still fail to see the original purpose of this patch in the 
> remoteproc core especially given that the core itself does not have 
> any callbacks. If the sole intention was to call the parent pdev's 
> callbacks, then I feel that state-machine is better managed within 
> that particular platform driver itself, as the sequencing/device 
> management can vary with different platform drivers.

The problem is that with Ingenic SoCs some clocks must be enabled in 
order to load the firmware, and the core doesn't give you an option to 
register a callback to be called before loading it. The first version 
of my patchset added .prepare/.unprepare callbacks to the struct 
rproc_ops, but the feedback from the maintainers was that I should do 
it via runtime PM. However, it was not possible to keep it contained in 
the driver, since again the core doesn't provide a "prepare" callback, 
so no place to call pm_runtime_get_sync(). So we settled with having 
runtime PM in the core without callbacks, which will trigger the 
runtime PM callbacks of the driver at the right moment.

Sorry if that caused you trouble.

Cheers,
-Paul
>>> 

>>> 
>>>> 
>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>> ---
>>>> 
>>>> Notes:
>>>>      v2-v4: No change
>>>>      v5: Move calls to prepare/unprepare to 
>>>> rproc_fw_boot/rproc_shutdown
>>>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>>>> \x7f\x7f\x7fcallbacks
>>>>      v7: Check return value of pm_runtime_get_sync()
>>>> 
>>>>   drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++++-
>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c 
>>>> \x7f\x7f\x7fb/drivers/remoteproc/remoteproc_core.c
>>>> index a7f96bc98406..e33d1ef27981 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -29,6 +29,7 @@
>>>>   #include <linux/devcoredump.h>
>>>>   #include <linux/rculist.h>
>>>>   #include <linux/remoteproc.h>
>>>> +#include <linux/pm_runtime.h>
>>>>   #include <linux/iommu.h>
>>>>   #include <linux/idr.h>
>>>>   #include <linux/elf.h>
>>>> @@ -1382,6 +1383,12 @@ static int rproc_fw_boot(struct rproc 
>>>> *rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>       if (ret)
>>>>           return ret;
>>>>   \x7f+    ret = pm_runtime_get_sync(dev);
>>>> +    if (ret < 0) {
>>>> +        dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>>       dev_info(dev, "Booting fw image %s, size %zd\n", name, 
>>>> fw->size);
>>>>   \x7f      /*
>>>> @@ -1391,7 +1398,7 @@ static int rproc_fw_boot(struct rproc 
>>>> *rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>       ret = rproc_enable_iommu(rproc);
>>>>       if (ret) {
>>>>           dev_err(dev, "can't enable iommu: %d\n", ret);
>>>> -        return ret;
>>>> +        goto put_pm_runtime;
>>>>       }
>>>>   \x7f      rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>>>> @@ -1435,6 +1442,8 @@ static int rproc_fw_boot(struct rproc 
>>>> *rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>       rproc->table_ptr = NULL;
>>>>   disable_iommu:
>>>>       rproc_disable_iommu(rproc);
>>>> +put_pm_runtime:
>>>> +    pm_runtime_put(dev);
>>>>       return ret;
>>>>   }
>>>>   \x7f@@ -1840,6 +1849,8 @@ void rproc_shutdown(struct rproc *rproc)
>>>>   \x7f      rproc_disable_iommu(rproc);
>>>>   \x7f+    pm_runtime_put(dev);
>>>> +
>>>>       /* Free the copy of the resource table */
>>>>       kfree(rproc->cached_table);
>>>>       rproc->cached_table = NULL;
>>>> @@ -2118,6 +2129,9 @@ struct rproc *rproc_alloc(struct device 
>>>> *dev, \x7f\x7f\x7fconst char *name,
>>>>   \x7f      rproc->state = RPROC_OFFLINE;
>>>>   \x7f+    pm_runtime_no_callbacks(&rproc->dev);
>>>> +    pm_runtime_enable(&rproc->dev);
>>>> +
>>>>       return rproc;
>>>>   }
>>>>   EXPORT_SYMBOL(rproc_alloc);
>>>> @@ -2133,6 +2147,7 @@ EXPORT_SYMBOL(rproc_alloc);
>>>>    */
>>>>   void rproc_free(struct rproc *rproc)
>>>>   {
>>>> +    pm_runtime_disable(&rproc->dev);
>>>>       put_device(&rproc->dev);
>>>>   }
>>>>   EXPORT_SYMBOL(rproc_free);
>>>> 
>>> 

Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM

[Suman Anna]

Hi Paul,

On 6/8/20 5:46 PM, Paul Cercueil wrote:
> Hi Suman,
> 
>>>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>>>> Call pm_runtime_get_sync() before the firmware is loaded, and
>>>>> pm_runtime_put() after the remote processor has been stopped.
>>>>>
>>>>> Even though the remoteproc device has no PM callbacks, this allows the
>>>>> parent device's PM callbacks to be properly called.
>>>>
>>>> I see this patch staged now for 5.8, and the latest -next branch 
>>>> has \x7f\x7fbroken the pm-runtime autosuspend feature we have in the 
>>>> OMAP \x7f\x7fremoteproc driver. See commit 5f31b232c674 ("remoteproc/omap: 
>>>> Add \x7f\x7fsupport for runtime auto-suspend/resume").
>>>>
>>>> What was the original purpose of this patch, because there can be 
>>>> \x7f\x7fdiffering backends across different SoCs.
>>>
>>> Did you try pm_suspend_ignore_children()? It looks like it was made 
>>> for \x7fyour use-case.
>>
>> Sorry for the delay in getting back. So, using 
>> pm_suspend_ignore_children() does fix my current issue.
>>
>> But I still fail to see the original purpose of this patch in the 
>> remoteproc core especially given that the core itself does not have 
>> any callbacks. If the sole intention was to call the parent pdev's 
>> callbacks, then I feel that state-machine is better managed within 
>> that particular platform driver itself, as the sequencing/device 
>> management can vary with different platform drivers.
> 
> The problem is that with Ingenic SoCs some clocks must be enabled in 
> order to load the firmware, and the core doesn't give you an option to 
> register a callback to be called before loading it.

Yep, I have similar usage in one of my remoteproc drivers (see 
keystone_remoteproc.c), and I think this all stems from the need to 
use/support loading into a processor's internal memories. My driver does 
leverage the pm-clks backend plugged into pm_runtime, so you won't see 
explicit calls on the clocks.

I guess the question is what exact PM features you are looking for with 
the Ingenic SoC. I do see you are using pm_runtime autosuspend, and your 
callbacks are managing the clocks, but reset is managed only in start/stop.

> The first version of 
> my patchset added .prepare/.unprepare callbacks to the struct rproc_ops, 
> but the feedback from the maintainers was that I should do it via 
> runtime PM. However, it was not possible to keep it contained in the 
> driver, since again the core doesn't provide a "prepare" callback, so no 
> place to call pm_runtime_get_sync(). 

FWIW, the .prepare/.unprepare callbacks is actually now part of the 
rproc core. Looks like multiple developers had a need for this, and this 
functionality went in at the same time as your driver :). Not sure if 
you looked up the prior patches, I leveraged the patch that Loic had 
submitted a long-time ago, and a revised version of it is now part of 
5.8-rc1.

So we settled with having runtime
> PM in the core without callbacks, which will trigger the runtime PM 
> callbacks of the driver at the right moment.

Looks like we can do some cleanup on the Ingenic SoC driver depending on 
the features you want.

regards
Suman

> 
> Sorry if that caused you trouble.
> 
> Cheers,
> -Paul
>>>>
> 
>>>>
>>>>>
>>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>      v2-v4: No change
>>>>>      v5: Move calls to prepare/unprepare to 
>>>>> rproc_fw_boot/rproc_shutdown
>>>>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>>>>> \x7f\x7f\x7fcallbacks
>>>>>      v7: Check return value of pm_runtime_get_sync()
>>>>>
>>>>>   drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++++-
>>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c 
>>>>> \x7f\x7f\x7fb/drivers/remoteproc/remoteproc_core.c
>>>>> index a7f96bc98406..e33d1ef27981 100644
>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>> @@ -29,6 +29,7 @@
>>>>>   #include <linux/devcoredump.h>
>>>>>   #include <linux/rculist.h>
>>>>>   #include <linux/remoteproc.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>>   #include <linux/iommu.h>
>>>>>   #include <linux/idr.h>
>>>>>   #include <linux/elf.h>
>>>>> @@ -1382,6 +1383,12 @@ static int rproc_fw_boot(struct rproc 
>>>>> *rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>>       if (ret)
>>>>>           return ret;
>>>>>   \x7f+    ret = pm_runtime_get_sync(dev);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>>       dev_info(dev, "Booting fw image %s, size %zd\n", name, 
>>>>> fw->size);
>>>>>   \x7f      /*
>>>>> @@ -1391,7 +1398,7 @@ static int rproc_fw_boot(struct rproc 
>>>>> *rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>>       ret = rproc_enable_iommu(rproc);
>>>>>       if (ret) {
>>>>>           dev_err(dev, "can't enable iommu: %d\n", ret);
>>>>> -        return ret;
>>>>> +        goto put_pm_runtime;
>>>>>       }
>>>>>   \x7f      rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>>>>> @@ -1435,6 +1442,8 @@ static int rproc_fw_boot(struct rproc 
>>>>> *rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>>       rproc->table_ptr = NULL;
>>>>>   disable_iommu:
>>>>>       rproc_disable_iommu(rproc);
>>>>> +put_pm_runtime:
>>>>> +    pm_runtime_put(dev);
>>>>>       return ret;
>>>>>   }
>>>>>   \x7f@@ -1840,6 +1849,8 @@ void rproc_shutdown(struct rproc *rproc)
>>>>>   \x7f      rproc_disable_iommu(rproc);
>>>>>   \x7f+    pm_runtime_put(dev);
>>>>> +
>>>>>       /* Free the copy of the resource table */
>>>>>       kfree(rproc->cached_table);
>>>>>       rproc->cached_table = NULL;
>>>>> @@ -2118,6 +2129,9 @@ struct rproc *rproc_alloc(struct device 
>>>>> *dev, \x7f\x7f\x7fconst char *name,
>>>>>   \x7f      rproc->state = RPROC_OFFLINE;
>>>>>   \x7f+    pm_runtime_no_callbacks(&rproc->dev);
>>>>> +    pm_runtime_enable(&rproc->dev);
>>>>> +
>>>>>       return rproc;
>>>>>   }
>>>>>   EXPORT_SYMBOL(rproc_alloc);
>>>>> @@ -2133,6 +2147,7 @@ EXPORT_SYMBOL(rproc_alloc);
>>>>>    */
>>>>>   void rproc_free(struct rproc *rproc)
>>>>>   {
>>>>> +    pm_runtime_disable(&rproc->dev);
>>>>>       put_device(&rproc->dev);
>>>>>   }
>>>>>   EXPORT_SYMBOL(rproc_free);
>>>>>
>>>>
> 
> 

Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM

[Paul Cercueil]

Hi,

Le lun. 8 juin 2020 à 18:10, Suman Anna <s-anna@ti.com> a écrit :
> Hi Paul,
> 
> On 6/8/20 5:46 PM, Paul Cercueil wrote:
>> Hi Suman,
>> 
>>>>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>>>>> Call pm_runtime_get_sync() before the firmware is loaded, and
>>>>>> pm_runtime_put() after the remote processor has been stopped.
>>>>>> 
>>>>>> Even though the remoteproc device has no PM callbacks, this 
>>>>>> allows the
>>>>>> parent device's PM callbacks to be properly called.
>>>>> 
>>>>> I see this patch staged now for 5.8, and the latest -next branch 
>>>>> \x7f\x7f\x7f\x7fhas \x7f\x7fbroken the pm-runtime autosuspend feature we have in 
>>>>> the \x7f\x7f\x7f\x7fOMAP \x7f\x7fremoteproc driver. See commit 5f31b232c674 
>>>>> ("remoteproc/omap: \x7f\x7f\x7f\x7fAdd \x7f\x7fsupport for runtime 
>>>>> auto-suspend/resume").
>>>>> 
>>>>> What was the original purpose of this patch, because there can be 
>>>>> \x7f\x7f\x7f\x7f\x7f\x7fdiffering backends across different SoCs.
>>>> 
>>>> Did you try pm_suspend_ignore_children()? It looks like it was 
>>>> made \x7f\x7f\x7ffor \x7fyour use-case.
>>> 
>>> Sorry for the delay in getting back. So, using 
>>> \x7f\x7fpm_suspend_ignore_children() does fix my current issue.
>>> 
>>> But I still fail to see the original purpose of this patch in the 
>>> \x7f\x7fremoteproc core especially given that the core itself does not 
>>> have \x7f\x7fany callbacks. If the sole intention was to call the parent 
>>> pdev's \x7f\x7fcallbacks, then I feel that state-machine is better 
>>> managed within \x7f\x7fthat particular platform driver itself, as the 
>>> sequencing/device \x7f\x7fmanagement can vary with different platform 
>>> drivers.
>> 
>> The problem is that with Ingenic SoCs some clocks must be enabled in 
>> \x7forder to load the firmware, and the core doesn't give you an option 
>> to \x7fregister a callback to be called before loading it.
> 
> Yep, I have similar usage in one of my remoteproc drivers (see 
> keystone_remoteproc.c), and I think this all stems from the need to 
> use/support loading into a processor's internal memories. My driver 
> does leverage the pm-clks backend plugged into pm_runtime, so you 
> won't see explicit calls on the clocks.
> 
> I guess the question is what exact PM features you are looking for 
> with the Ingenic SoC. I do see you are using pm_runtime autosuspend, 
> and your callbacks are managing the clocks, but reset is managed only 
> in start/stop.
> 
>> The first version of \x7fmy patchset added .prepare/.unprepare 
>> callbacks to the struct rproc_ops, \x7fbut the feedback from the 
>> maintainers was that I should do it via \x7fruntime PM. However, it was 
>> not possible to keep it contained in the \x7fdriver, since again the 
>> core doesn't provide a "prepare" callback, so no \x7fplace to call 
>> pm_runtime_get_sync().
> FWIW, the .prepare/.unprepare callbacks is actually now part of the 
> rproc core. Looks like multiple developers had a need for this, and 
> this functionality went in at the same time as your driver :). Not 
> sure if you looked up the prior patches, I leveraged the patch that 
> Loic had submitted a long-time ago, and a revised version of it is 
> now part of 5.8-rc1.

WTF maintainers, you refuse my patchset for adding a 
.prepare/.unprepare, ask me to do it via runtime PM, then merge another 
patchset that adds these callback. At least be constant in your 
decisions.

Anyway, now we have two methods added to linux-next for doing the exact 
same thing. What should we do about it?

-Paul

> So we settled with having runtime
>> PM in the core without callbacks, which will trigger the runtime PM 
>> \x7fcallbacks of the driver at the right moment.
> 
> Looks like we can do some cleanup on the Ingenic SoC driver depending 
> on the features you want.
> 
> regards
> Suman
> 
>> 
>> Sorry if that caused you trouble.
>> 
>> Cheers,
>> -Paul
>>>>> 
>> 
>>>>> 
>>>>>> 
>>>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>>> ---
>>>>>> 
>>>>>> Notes:
>>>>>>      v2-v4: No change
>>>>>>      v5: Move calls to prepare/unprepare to 
>>>>>> \x7f\x7f\x7f\x7f\x7frproc_fw_boot/rproc_shutdown
>>>>>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fcallbacks
>>>>>>      v7: Check return value of pm_runtime_get_sync()
>>>>>> 
>>>>>>   drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++++-
>>>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c 
>>>>>> \x7f\x7f\x7f\x7f\x7f\x7f\x7f\x7fb/drivers/remoteproc/remoteproc_core.c
>>>>>> index a7f96bc98406..e33d1ef27981 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>>> @@ -29,6 +29,7 @@
>>>>>>   #include <linux/devcoredump.h>
>>>>>>   #include <linux/rculist.h>
>>>>>>   #include <linux/remoteproc.h>
>>>>>> +#include <linux/pm_runtime.h>
>>>>>>   #include <linux/iommu.h>
>>>>>>   #include <linux/idr.h>
>>>>>>   #include <linux/elf.h>
>>>>>> @@ -1382,6 +1383,12 @@ static int rproc_fw_boot(struct rproc 
>>>>>> \x7f\x7f\x7f\x7f\x7f*rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>>>       if (ret)
>>>>>>           return ret;
>>>>>>   \x7f+    ret = pm_runtime_get_sync(dev);
>>>>>> +    if (ret < 0) {
>>>>>> +        dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
>>>>>> +        return ret;
>>>>>> +    }
>>>>>> +
>>>>>>       dev_info(dev, "Booting fw image %s, size %zd\n", name, 
>>>>>> \x7f\x7f\x7f\x7f\x7ffw->size);
>>>>>>   \x7f      /*
>>>>>> @@ -1391,7 +1398,7 @@ static int rproc_fw_boot(struct rproc 
>>>>>> \x7f\x7f\x7f\x7f\x7f*rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>>>       ret = rproc_enable_iommu(rproc);
>>>>>>       if (ret) {
>>>>>>           dev_err(dev, "can't enable iommu: %d\n", ret);
>>>>>> -        return ret;
>>>>>> +        goto put_pm_runtime;
>>>>>>       }
>>>>>>   \x7f      rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>>>>>> @@ -1435,6 +1442,8 @@ static int rproc_fw_boot(struct rproc 
>>>>>> \x7f\x7f\x7f\x7f\x7f*rproc, \x7f\x7f\x7fconst struct firmware *fw)
>>>>>>       rproc->table_ptr = NULL;
>>>>>>   disable_iommu:
>>>>>>       rproc_disable_iommu(rproc);
>>>>>> +put_pm_runtime:
>>>>>> +    pm_runtime_put(dev);
>>>>>>       return ret;
>>>>>>   }
>>>>>>   \x7f@@ -1840,6 +1849,8 @@ void rproc_shutdown(struct rproc *rproc)
>>>>>>   \x7f      rproc_disable_iommu(rproc);
>>>>>>   \x7f+    pm_runtime_put(dev);
>>>>>> +
>>>>>>       /* Free the copy of the resource table */
>>>>>>       kfree(rproc->cached_table);
>>>>>>       rproc->cached_table = NULL;
>>>>>> @@ -2118,6 +2129,9 @@ struct rproc *rproc_alloc(struct device 
>>>>>> \x7f\x7f\x7f\x7f\x7f*dev, \x7f\x7f\x7fconst char *name,
>>>>>>   \x7f      rproc->state = RPROC_OFFLINE;
>>>>>>   \x7f+    pm_runtime_no_callbacks(&rproc->dev);
>>>>>> +    pm_runtime_enable(&rproc->dev);
>>>>>> +
>>>>>>       return rproc;
>>>>>>   }
>>>>>>   EXPORT_SYMBOL(rproc_alloc);
>>>>>> @@ -2133,6 +2147,7 @@ EXPORT_SYMBOL(rproc_alloc);
>>>>>>    */
>>>>>>   void rproc_free(struct rproc *rproc)
>>>>>>   {
>>>>>> +    pm_runtime_disable(&rproc->dev);
>>>>>>       put_device(&rproc->dev);
>>>>>>   }
>>>>>>   EXPORT_SYMBOL(rproc_free);
>>>>>> 
>>>>> 
>> 
>> 
> 

Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM

[Bjorn Andersson]

On Wed 10 Jun 02:40 PDT 2020, Paul Cercueil wrote:

> Hi,
> 
> Le lun. 8 juin 2020 à 18:10, Suman Anna <s-anna@ti.com> a écrit :
> > Hi Paul,
> > 
> > On 6/8/20 5:46 PM, Paul Cercueil wrote:
> > > Hi Suman,
> > > 
> > > > > > On 5/15/20 5:43 AM, Paul Cercueil wrote:
> > > > > > > Call pm_runtime_get_sync() before the firmware is loaded, and
> > > > > > > pm_runtime_put() after the remote processor has been stopped.
> > > > > > > 
> > > > > > > Even though the remoteproc device has no PM
> > > > > > > callbacks, this allows the
> > > > > > > parent device's PM callbacks to be properly called.
> > > > > > 
> > > > > > I see this patch staged now for 5.8, and the latest
> > > > > > -next branch \x7f\x7f\x7f\x7fhas \x7f\x7fbroken the pm-runtime autosuspend
> > > > > > feature we have in the \x7f\x7f\x7f\x7fOMAP \x7f\x7fremoteproc driver. See
> > > > > > commit 5f31b232c674 ("remoteproc/omap: \x7f\x7f\x7f\x7fAdd \x7f\x7fsupport
> > > > > > for runtime auto-suspend/resume").
> > > > > > 
> > > > > > What was the original purpose of this patch, because
> > > > > > there can be \x7f\x7f\x7f\x7f\x7f\x7fdiffering backends across different
> > > > > > SoCs.
> > > > > 
> > > > > Did you try pm_suspend_ignore_children()? It looks like it
> > > > > was made \x7f\x7f\x7ffor \x7fyour use-case.
> > > > 
> > > > Sorry for the delay in getting back. So, using
> > > > \x7f\x7fpm_suspend_ignore_children() does fix my current issue.
> > > > 
> > > > But I still fail to see the original purpose of this patch in
> > > > the \x7f\x7fremoteproc core especially given that the core itself does
> > > > not have \x7f\x7fany callbacks. If the sole intention was to call the
> > > > parent pdev's \x7f\x7fcallbacks, then I feel that state-machine is
> > > > better managed within \x7f\x7fthat particular platform driver itself,
> > > > as the sequencing/device \x7f\x7fmanagement can vary with different
> > > > platform drivers.
> > > 
> > > The problem is that with Ingenic SoCs some clocks must be enabled in
> > > \x7forder to load the firmware, and the core doesn't give you an option
> > > to \x7fregister a callback to be called before loading it.
> > 
> > Yep, I have similar usage in one of my remoteproc drivers (see
> > keystone_remoteproc.c), and I think this all stems from the need to
> > use/support loading into a processor's internal memories. My driver does
> > leverage the pm-clks backend plugged into pm_runtime, so you won't see
> > explicit calls on the clocks.
> > 
> > I guess the question is what exact PM features you are looking for with
> > the Ingenic SoC. I do see you are using pm_runtime autosuspend, and your
> > callbacks are managing the clocks, but reset is managed only in
> > start/stop.
> > 
> > > The first version of \x7fmy patchset added .prepare/.unprepare
> > > callbacks to the struct rproc_ops, \x7fbut the feedback from the
> > > maintainers was that I should do it via \x7fruntime PM. However, it was
> > > not possible to keep it contained in the \x7fdriver, since again the
> > > core doesn't provide a "prepare" callback, so no \x7fplace to call
> > > pm_runtime_get_sync().
> > FWIW, the .prepare/.unprepare callbacks is actually now part of the
> > rproc core. Looks like multiple developers had a need for this, and this
> > functionality went in at the same time as your driver :). Not sure if
> > you looked up the prior patches, I leveraged the patch that Loic had
> > submitted a long-time ago, and a revised version of it is now part of
> > 5.8-rc1.
> 
> WTF maintainers, you refuse my patchset for adding a .prepare/.unprepare,
> ask me to do it via runtime PM, then merge another patchset that adds these
> callback. At least be constant in your decisions.
> 

Sorry, I missed this when applying the two patches, but you're of course
right.

> Anyway, now we have two methods added to linux-next for doing the exact same
> thing. What should we do about it?
> 

I like the pm_runtime approach and as it was Arnaud that asked you to
change it, perhaps he and Loic can agree on updating the ST driver so we
can drop the prepare/unprepare ops again?

Regards,
Bjorn

Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM

[Suman Anna]

On 6/10/20 11:39 PM, Bjorn Andersson wrote:
> On Wed 10 Jun 02:40 PDT 2020, Paul Cercueil wrote:
> 
>> Hi,
>>
>> Le lun. 8 juin 2020 à 18:10, Suman Anna <s-anna@ti.com> a écrit :
>>> Hi Paul,
>>>
>>> On 6/8/20 5:46 PM, Paul Cercueil wrote:
>>>> Hi Suman,
>>>>
>>>>>>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>>>>>>> Call pm_runtime_get_sync() before the firmware is loaded, and
>>>>>>>> pm_runtime_put() after the remote processor has been stopped.
>>>>>>>>
>>>>>>>> Even though the remoteproc device has no PM
>>>>>>>> callbacks, this allows the
>>>>>>>> parent device's PM callbacks to be properly called.
>>>>>>>
>>>>>>> I see this patch staged now for 5.8, and the latest
>>>>>>> -next branch \x7f\x7f\x7f\x7fhas \x7f\x7fbroken the pm-runtime autosuspend
>>>>>>> feature we have in the \x7f\x7f\x7f\x7fOMAP \x7f\x7fremoteproc driver. See
>>>>>>> commit 5f31b232c674 ("remoteproc/omap: \x7f\x7f\x7f\x7fAdd \x7f\x7fsupport
>>>>>>> for runtime auto-suspend/resume").
>>>>>>>
>>>>>>> What was the original purpose of this patch, because
>>>>>>> there can be \x7f\x7f\x7f\x7f\x7f\x7fdiffering backends across different
>>>>>>> SoCs.
>>>>>>
>>>>>> Did you try pm_suspend_ignore_children()? It looks like it
>>>>>> was made \x7f\x7f\x7ffor \x7fyour use-case.
>>>>>
>>>>> Sorry for the delay in getting back. So, using
>>>>> \x7f\x7fpm_suspend_ignore_children() does fix my current issue.
>>>>>
>>>>> But I still fail to see the original purpose of this patch in
>>>>> the \x7f\x7fremoteproc core especially given that the core itself does
>>>>> not have \x7f\x7fany callbacks. If the sole intention was to call the
>>>>> parent pdev's \x7f\x7fcallbacks, then I feel that state-machine is
>>>>> better managed within \x7f\x7fthat particular platform driver itself,
>>>>> as the sequencing/device \x7f\x7fmanagement can vary with different
>>>>> platform drivers.
>>>>
>>>> The problem is that with Ingenic SoCs some clocks must be enabled in
>>>> \x7forder to load the firmware, and the core doesn't give you an option
>>>> to \x7fregister a callback to be called before loading it.
>>>
>>> Yep, I have similar usage in one of my remoteproc drivers (see
>>> keystone_remoteproc.c), and I think this all stems from the need to
>>> use/support loading into a processor's internal memories. My driver does
>>> leverage the pm-clks backend plugged into pm_runtime, so you won't see
>>> explicit calls on the clocks.
>>>
>>> I guess the question is what exact PM features you are looking for with
>>> the Ingenic SoC. I do see you are using pm_runtime autosuspend, and your
>>> callbacks are managing the clocks, but reset is managed only in
>>> start/stop.
>>>
>>>> The first version of \x7fmy patchset added .prepare/.unprepare
>>>> callbacks to the struct rproc_ops, \x7fbut the feedback from the
>>>> maintainers was that I should do it via \x7fruntime PM. However, it was
>>>> not possible to keep it contained in the \x7fdriver, since again the
>>>> core doesn't provide a "prepare" callback, so no \x7fplace to call
>>>> pm_runtime_get_sync().
>>> FWIW, the .prepare/.unprepare callbacks is actually now part of the
>>> rproc core. Looks like multiple developers had a need for this, and this
>>> functionality went in at the same time as your driver :). Not sure if
>>> you looked up the prior patches, I leveraged the patch that Loic had
>>> submitted a long-time ago, and a revised version of it is now part of
>>> 5.8-rc1.
>>
>> WTF maintainers, you refuse my patchset for adding a .prepare/.unprepare,
>> ask me to do it via runtime PM, then merge another patchset that adds these
>> callback. At least be constant in your decisions.
>>
> 
> Sorry, I missed this when applying the two patches, but you're of course
> right.
> 
>> Anyway, now we have two methods added to linux-next for doing the exact same
>> thing. What should we do about it?
>>
> 
> I like the pm_runtime approach and as it was Arnaud that asked you to
> change it, perhaps he and Loic can agree on updating the ST driver so we
> can drop the prepare/unprepare ops again?

These callbacks were added primarily in preparation for the TI K3 rproc 
drivers, not just ST (the patch was resurrected from a very old patch 
from Loic).

I still think prepare/unprepare is actually better suited to scale well 
for the long term. This pm_runtime logic will now make the early-boot 
scenarios complicated, as you would have to match its status, but all 
actual operations are on the actual parent remoteproc platform device 
and not the child remoteproc device. I think it serves to mess up the 
state-machines of different platform drivers due to additional refcounts 
acquired and maybe performing some operations out of sequence to what a 
platform driver wants esp. if there is automated backend usage like 
genpd, pm_clks etc. I am yet to review Mathieu's latest MCU sync series, 
but the concept of different sync_ops already scales w.r.t the 
prepare/unprepare.

As for my K3 drivers, the callbacks are doing more than just turning on 
clocks, as the R5Fs in general as a complex power-on sequence. I do not 
have remoteproc auto-suspend atm on the K3 drivers, but that typically 
means shutting down and restoring the core and would involve all the 
hardware-specific sequences, so the rpm callback implementations will be 
more than just clocks.

I looked through the patch history on the Ingenic remoteproc driver, and 
the only reason for either of runtime pm usage or prepare/unprepare ops 
usage is to ensure that clocks do not stay enabled in the case the 
processor is not loaded/started. The driver is using auto-boot, so when 
it probes, in general we expect the remoteproc to be running. So, the 
only failure case is if there is no firmware. Otherwise, Paul could have 
just used clk_bulk API in probe and remove.

Anyway, I will provide some additional review comments on the pm_runtime 
usage within the Ingenic rproc driver.

regards
Suman

Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver

[Suman Anna]

Hi Paul,

On 5/15/20 5:43 AM, Paul Cercueil wrote:
> 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.

I have a few comments w.r.t pm_runtime usage in this driver.

> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 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()
>      v5: No change
>      v6: Instead of prepare/unprepare callbacks, use PM runtime callbacks
>      v7: - Remove use of of_match_ptr()
>      	- Move Kconfig symbol so that it's in alphabetical order
>      	- Add missing doc for private structure field aux_base
>      	- Don't check for (len <= 0) in da_to_va()
>      	- Add missing \n in dev_info/dev_err messages
> 
>   drivers/remoteproc/Kconfig         |   9 +
>   drivers/remoteproc/Makefile        |   1 +
>   drivers/remoteproc/ingenic_rproc.c | 280 +++++++++++++++++++++++++++++
>   3 files changed, 290 insertions(+)
>   create mode 100644 drivers/remoteproc/ingenic_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index fbaed079b299..c4d1731295eb 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -23,6 +23,15 @@ config IMX_REMOTEPROC
>   
>   	  It's safe to say N here.
>   
> +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.
> +
>   config MTK_SCP
>   	tristate "Mediatek SCP support"
>   	depends on ARCH_MEDIATEK
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 0effd3825035..e8b886e511f0 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>   obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>   obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
> diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
> new file mode 100644
> index 000000000000..189020d77b25
> --- /dev/null
> +++ b/drivers/remoteproc/ingenic_rproc.c
> @@ -0,0 +1,280 @@
> +// 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/pm_runtime.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
> + * @aux_base: raw pointer to the AUX interface registers
> + * @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_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, size_t len)
> +{
> +	struct vpu *vpu = rproc->priv;
> +	void __iomem *va = NULL;
> +	unsigned int i;
> +
> +	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 = {
> +	.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 void ingenic_rproc_disable_clks(void *data)
> +{
> +	struct vpu *vpu = data;
> +
> +	pm_runtime_resume(vpu->dev);
> +	pm_runtime_disable(vpu->dev);
> +
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
> +}
> +
> +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;
> +	platform_set_drvdata(pdev, vpu);
> +
> +	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\n");
> +		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\n");
> +			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\n");
> +		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\n");
> +		return ret;
> +	}
> +
> +	disable_irq(vpu->irq);
> +
> +	/* 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(dev, "Unable to start clocks\n");
> +		return ret;
> +	}

You are enabling the clocks directly here and also trying to manage them 
through pm_runtime callbacks again.

> +
> +	pm_runtime_irq_safe(dev);

Nothing wrong with this, but this does take an additional reference 
count on the parent device (a bus device for you??), and also implies 
that your clk driver code can all run in atomic context so unless you 
have a strong reason, it is safe to drop this.

> +	pm_runtime_set_active(dev);

The get_sync below would have been sufficient if you had either limited 
the clk API above to just clk_prepare, or you could have moved the whole 
clk API above into your runtime resume callback.

> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);

If the intention was to increment the usage count with the above 
sequence, pm_runtime_get_noresume() is better. But dropping all of the 
above and just using get_sync would have been sufficient.

> +	pm_runtime_use_autosuspend(dev);

I don't see any setting of the autosuspend delay (default value is 0). 
So, you might have as well just not used this at all, and just used 
pm_runtime_put() below.

> +
> +	ret = devm_add_action_or_reset(dev, ingenic_rproc_disable_clks, vpu);
> +	if (ret) {
> +		dev_err(dev, "Unable to register action\n");
> +		goto out_pm_put;
> +	}
> +
> +	ret = devm_rproc_add(dev, rproc);
> +	if (ret) {
> +		dev_err(dev, "Failed to register remote processor\n");
> +		goto out_pm_put;
> +	}

You are using auto-boot, so the firmware loading is an asynchronous 
event and most probably you would run through below sequence first, and 
end up disabling the clocks with an incorrect rpm status.

> +
> +out_pm_put:
> +	pm_runtime_put_autosuspend(dev);

And finally, with the remoteproc core rpm patch, this would all have 
been unnecessary.

> +
> +	return ret;
> +}
> +
> +static const struct of_device_id ingenic_rproc_of_matches[] = {
> +	{ .compatible = "ingenic,jz4770-vpu-rproc", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
> +
> +static int __maybe_unused ingenic_rproc_suspend(struct device *dev)
> +{
> +	struct vpu *vpu = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ingenic_rproc_resume(struct device *dev)
> +{
> +	struct vpu *vpu = dev_get_drvdata(dev);
> +
> +	return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
> +}
> +
> +static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
> +	SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, ingenic_rproc_resume, NULL)
> +};
> +
> +static struct platform_driver ingenic_rproc_driver = {
> +	.probe = ingenic_rproc_probe,
> +	.driver = {
> +		.name = "ingenic-vpu",
> +#ifdef CONFIG_PM

Not sure why you would want to maintain this conditional, because 
runtime_pm is a core dependency now for your driver to work properly.

regards
Suman

> +		.pm = &ingenic_rproc_pm,
> +#endif
> +		.of_match_table = 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");
> 

Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver

[Paul Cercueil]

Hi Suman,

Le jeu. 11 juin 2020 à 16:47, Suman Anna <s-anna@ti.com> a écrit :
> Hi Paul,
> 
> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>> 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.
> 
> I have a few comments w.r.t pm_runtime usage in this driver.
> 
>> 
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> 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()
>>      v5: No change
>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>> callbacks
>>      v7: - Remove use of of_match_ptr()
>>      	- Move Kconfig symbol so that it's in alphabetical order
>>      	- Add missing doc for private structure field aux_base
>>      	- Don't check for (len <= 0) in da_to_va()
>>      	- Add missing \n in dev_info/dev_err messages
>> 
>>   drivers/remoteproc/Kconfig         |   9 +
>>   drivers/remoteproc/Makefile        |   1 +
>>   drivers/remoteproc/ingenic_rproc.c | 280 
>> +++++++++++++++++++++++++++++
>>   3 files changed, 290 insertions(+)
>>   create mode 100644 drivers/remoteproc/ingenic_rproc.c
>> 
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index fbaed079b299..c4d1731295eb 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -23,6 +23,15 @@ config IMX_REMOTEPROC
>>   \x7f  	  It's safe to say N here.
>>   \x7f+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.
>> +
>>   config MTK_SCP
>>   	tristate "Mediatek SCP support"
>>   	depends on ARCH_MEDIATEK
>> diff --git a/drivers/remoteproc/Makefile 
>> b/drivers/remoteproc/Makefile
>> index 0effd3825035..e8b886e511f0 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_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>>   obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>>   obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
>> diff --git a/drivers/remoteproc/ingenic_rproc.c 
>> b/drivers/remoteproc/ingenic_rproc.c
>> new file mode 100644
>> index 000000000000..189020d77b25
>> --- /dev/null
>> +++ b/drivers/remoteproc/ingenic_rproc.c
>> @@ -0,0 +1,280 @@
>> +// 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/pm_runtime.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
>> + * @aux_base: raw pointer to the AUX interface registers
>> + * @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_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, 
>> size_t len)
>> +{
>> +	struct vpu *vpu = rproc->priv;
>> +	void __iomem *va = NULL;
>> +	unsigned int i;
>> +
>> +	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 = {
>> +	.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 void ingenic_rproc_disable_clks(void *data)
>> +{
>> +	struct vpu *vpu = data;
>> +
>> +	pm_runtime_resume(vpu->dev);
>> +	pm_runtime_disable(vpu->dev);
>> +
>> +	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
>> +}
>> +
>> +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;
>> +	platform_set_drvdata(pdev, vpu);
>> +
>> +	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\n");
>> +		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\n");
>> +			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\n");
>> +		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\n");
>> +		return ret;
>> +	}
>> +
>> +	disable_irq(vpu->irq);
>> +
>> +	/* 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(dev, "Unable to start clocks\n");
>> +		return ret;
>> +	}
> 
> You are enabling the clocks directly here and also trying to manage 
> them through pm_runtime callbacks again.

Yes. The clocks need to be enabled in the probe.

>> +
>> +	pm_runtime_irq_safe(dev);
> 
> Nothing wrong with this, but this does take an additional reference 
> count on the parent device (a bus device for you??), and also implies 
> that your clk driver code can all run in atomic context so unless you 
> have a strong reason, it is safe to drop this.

The clock driver code can run in atomic context, it is guaranteed by 
the clk API.

> 
>> +	pm_runtime_set_active(dev);
> 
> The get_sync below would have been sufficient if you had either 
> limited the clk API above to just clk_prepare, or you could have 
> moved the whole clk API above into your runtime resume callback.

You assume that pm_runtime_get() will enable the clocks, but that's 
only true if CONFIG_PM is set.

The reason the clocks are enabled in the probe, and 
pm_runtime_set_active() is called, is that it works whether or not 
CONFIG_PM is set.

>> +	pm_runtime_enable(dev);
>> +	pm_runtime_get_sync(dev);
> 
> If the intention was to increment the usage count with the above 
> sequence, pm_runtime_get_noresume() is better. But dropping all of 
> the above and just using get_sync would have been sufficient.
> 
>> +	pm_runtime_use_autosuspend(dev);
> 
> I don't see any setting of the autosuspend delay (default value is 
> 0). So, you might have as well just not used this at all, and just 
> used pm_runtime_put() below.

Autosuspend delay value can be set from userspace.

>> +
>> +	ret = devm_add_action_or_reset(dev, ingenic_rproc_disable_clks, 
>> vpu);
>> +	if (ret) {
>> +		dev_err(dev, "Unable to register action\n");
>> +		goto out_pm_put;
>> +	}
>> +
>> +	ret = devm_rproc_add(dev, rproc);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register remote processor\n");
>> +		goto out_pm_put;
>> +	}
> 
> You are using auto-boot, so the firmware loading is an asynchronous 
> event and most probably you would run through below sequence first, 
> and end up disabling the clocks with an incorrect rpm status.

The driver can auto-load the firmware, but that does not mean it will. 
We actually don't do that, and load a task-specific firmware onto the 
remote processor on demand.

>> +
>> +out_pm_put:
>> +	pm_runtime_put_autosuspend(dev);
> 
> And finally, with the remoteproc core rpm patch, this would all have 
> been unnecessary.
> 
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id ingenic_rproc_of_matches[] = {
>> +	{ .compatible = "ingenic,jz4770-vpu-rproc", },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
>> +
>> +static int __maybe_unused ingenic_rproc_suspend(struct device *dev)
>> +{
>> +	struct vpu *vpu = dev_get_drvdata(dev);
>> +
>> +	clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __maybe_unused ingenic_rproc_resume(struct device *dev)
>> +{
>> +	struct vpu *vpu = dev_get_drvdata(dev);
>> +
>> +	return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
>> +}
>> +
>> +static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
>> +	SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, ingenic_rproc_resume, 
>> NULL)
>> +};
>> +
>> +static struct platform_driver ingenic_rproc_driver = {
>> +	.probe = ingenic_rproc_probe,
>> +	.driver = {
>> +		.name = "ingenic-vpu",
>> +#ifdef CONFIG_PM
> 
> Not sure why you would want to maintain this conditional, because 
> runtime_pm is a core dependency now for your driver to work properly.

No, it is not - the driver works perfectly fine with CONFIG_PM being 
disabled, and having a hardcoded dependency on CONFIG_PM is not 
something we want.

Cheers,
-Paul

> regards
> Suman
> 
>> +		.pm = &ingenic_rproc_pm,
>> +#endif
>> +		.of_match_table = 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");
>> 
> 

Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver

[Suman Anna]

Hi Paul,

On 6/11/20 5:21 PM, Paul Cercueil wrote:
> Hi Suman,
> 
> Le jeu. 11 juin 2020 à 16:47, Suman Anna <s-anna@ti.com> a écrit :
>> Hi Paul,
>>
>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>> 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.
>>
>> I have a few comments w.r.t pm_runtime usage in this driver.
>>
>>>
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>> 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()
>>>      v5: No change
>>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>>> callbacks
>>>      v7: - Remove use of of_match_ptr()
>>>          - Move Kconfig symbol so that it's in alphabetical order
>>>          - Add missing doc for private structure field aux_base
>>>          - Don't check for (len <= 0) in da_to_va()
>>>          - Add missing \n in dev_info/dev_err messages
>>>
>>>   drivers/remoteproc/Kconfig         |   9 +
>>>   drivers/remoteproc/Makefile        |   1 +
>>>   drivers/remoteproc/ingenic_rproc.c | 280 +++++++++++++++++++++++++++++
>>>   3 files changed, 290 insertions(+)
>>>   create mode 100644 drivers/remoteproc/ingenic_rproc.c
>>>
>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>> index fbaed079b299..c4d1731295eb 100644
>>> --- a/drivers/remoteproc/Kconfig
>>> +++ b/drivers/remoteproc/Kconfig
>>> @@ -23,6 +23,15 @@ config IMX_REMOTEPROC
>>>   \x7f        It's safe to say N here.
>>>   \x7f+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.
>>> +
>>>   config MTK_SCP
>>>       tristate "Mediatek SCP support"
>>>       depends on ARCH_MEDIATEK
>>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>>> index 0effd3825035..e8b886e511f0 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_MTK_SCP)            += mtk_scp.o mtk_scp_ipi.o
>>>   obj-$(CONFIG_OMAP_REMOTEPROC)        += omap_remoteproc.o
>>>   obj-$(CONFIG_WKUP_M3_RPROC)        += wkup_m3_rproc.o
>>> diff --git a/drivers/remoteproc/ingenic_rproc.c 
>>> b/drivers/remoteproc/ingenic_rproc.c
>>> new file mode 100644
>>> index 000000000000..189020d77b25
>>> --- /dev/null
>>> +++ b/drivers/remoteproc/ingenic_rproc.c
>>> @@ -0,0 +1,280 @@
>>> +// 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/pm_runtime.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
>>> + * @aux_base: raw pointer to the AUX interface registers
>>> + * @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_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, 
>>> size_t len)
>>> +{
>>> +    struct vpu *vpu = rproc->priv;
>>> +    void __iomem *va = NULL;
>>> +    unsigned int i;
>>> +
>>> +    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 = {
>>> +    .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 void ingenic_rproc_disable_clks(void *data)
>>> +{
>>> +    struct vpu *vpu = data;
>>> +
>>> +    pm_runtime_resume(vpu->dev);
>>> +    pm_runtime_disable(vpu->dev);
>>> +
>>> +    clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
>>> +}
>>> +
>>> +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;
>>> +    platform_set_drvdata(pdev, vpu);
>>> +
>>> +    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\n");
>>> +        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\n");
>>> +            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\n");
>>> +        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\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    disable_irq(vpu->irq);
>>> +
>>> +    /* 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(dev, "Unable to start clocks\n");
>>> +        return ret;
>>> +    }
>>
>> You are enabling the clocks directly here and also trying to manage 
>> them through pm_runtime callbacks again.
> 
> Yes. The clocks need to be enabled in the probe.

For the preferred non CONFIG_PM case now and lack of prepare/unprepare().

> 
>>> +
>>> +    pm_runtime_irq_safe(dev);
>>
>> Nothing wrong with this, but this does take an additional reference 
>> count on the parent device (a bus device for you??), and also implies 
>> that your clk driver code can all run in atomic context so unless you 
>> have a strong reason, it is safe to drop this.
> 
> The clock driver code can run in atomic context, it is guaranteed by the 
> clk API.

OK.

> 
>>
>>> +    pm_runtime_set_active(dev);
>>
>> The get_sync below would have been sufficient if you had either 
>> limited the clk API above to just clk_prepare, or you could have moved 
>> the whole clk API above into your runtime resume callback.
> 
> You assume that pm_runtime_get() will enable the clocks, but that's only 
> true if CONFIG_PM is set.
> 
> The reason the clocks are enabled in the probe, and 
> pm_runtime_set_active() is called, is that it works whether or not 
> CONFIG_PM is set.

As I said, if the intention is to reflect the clocks active state in rpm 
status, then pm_runtime_get_noresume() does the job for you instead of 
get_sync(). pm_runtime_get_sync() typically does 3 things - increment 
the usage count, invoke your callbacks (enable clocks for you), and sets 
the status to active, with the last two steps optional depending on 
usage count.

> 
>>> +    pm_runtime_enable(dev);
>>> +    pm_runtime_get_sync(dev);
>>
>> If the intention was to increment the usage count with the above 
>> sequence, pm_runtime_get_noresume() is better. But dropping all of the 
>> above and just using get_sync would have been sufficient.
>>
>>> +    pm_runtime_use_autosuspend(dev);
>>
>> I don't see any setting of the autosuspend delay (default value is 0). 
>> So, you might have as well just not used this at all, and just used 
>> pm_runtime_put() below.
> 
> Autosuspend delay value can be set from userspace.

Yes, but I don't see a specific purpose for it in your driver. Either 
you have remoteproc running (and so clocks enabled always), or you don't 
have a firmware loaded and want clocks disabled (not sure you would want 
to waste power for certain of amount of time).

> 
>>> +
>>> +    ret = devm_add_action_or_reset(dev, ingenic_rproc_disable_clks, 
>>> vpu);
>>> +    if (ret) {
>>> +        dev_err(dev, "Unable to register action\n");
>>> +        goto out_pm_put;
>>> +    }
>>> +
>>> +    ret = devm_rproc_add(dev, rproc);
>>> +    if (ret) {
>>> +        dev_err(dev, "Failed to register remote processor\n");
>>> +        goto out_pm_put;
>>> +    }
>>
>> You are using auto-boot, so the firmware loading is an asynchronous 
>> event and most probably you would run through below sequence first, 
>> and end up disabling the clocks with an incorrect rpm status.
> 
> The driver can auto-load the firmware, but that does not mean it will. 
> We actually don't do that, and load a task-specific firmware onto the 
> remote processor on demand.

Yeah OK, depends on what is preferred by default. If it is more standard 
practise that the remoteproc is booted by userspace always, then I 
suggest setting auto_boot as false. But nothing wrong with expecting to 
boot by default with a starting firmware.

> 
>>> +
>>> +out_pm_put:
>>> +    pm_runtime_put_autosuspend(dev);
>>
>> And finally, with the remoteproc core rpm patch, this would all have 
>> been unnecessary.
>>
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static const struct of_device_id ingenic_rproc_of_matches[] = {
>>> +    { .compatible = "ingenic,jz4770-vpu-rproc", },
>>> +    {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
>>> +
>>> +static int __maybe_unused ingenic_rproc_suspend(struct device *dev)
>>> +{
>>> +    struct vpu *vpu = dev_get_drvdata(dev);
>>> +
>>> +    clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int __maybe_unused ingenic_rproc_resume(struct device *dev)
>>> +{
>>> +    struct vpu *vpu = dev_get_drvdata(dev);
>>> +
>>> +    return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
>>> +}
>>> +
>>> +static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
>>> +    SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, ingenic_rproc_resume, 
>>> NULL)
>>> +};
>>> +
>>> +static struct platform_driver ingenic_rproc_driver = {
>>> +    .probe = ingenic_rproc_probe,
>>> +    .driver = {
>>> +        .name = "ingenic-vpu",
>>> +#ifdef CONFIG_PM
>>
>> Not sure why you would want to maintain this conditional, because 
>> runtime_pm is a core dependency now for your driver to work properly.
> 
> No, it is not - the driver works perfectly fine with CONFIG_PM being 
> disabled, and having a hardcoded dependency on CONFIG_PM is not 
> something we want.

OK, so if IIUC, in general CONFIG_PM is not a typical usage for your 
MIPS platforms. Your driver is the first non-ARM remoteproc driver :), 
CONFIG_PM is almost a given on most ARM platforms.

So, I fail to see how your clocks can stay disabled then when 
CONFIG_PM=n if the remoteproc fails to load with the current code, which 
was your primary argument for using prepare/unprepare (based on comments 
on prior versions). It looks to me that your needs are indeed better 
suited with the prepare/unprepare ops as in your initial series.

And in the case of CONFIG_PM=y, you have three levels of code that 
enables the clocks (the bare clk API, the pm_runtime_get in probe, and 
the pm_runtime_get in the remoteproc core). Depending on the rpm status, 
it may or may not invoke the callbacks.

regards
Suman

> 
> Cheers,
> -Paul
> 
>> regards
>> Suman
>>
>>> +        .pm = &ingenic_rproc_pm,
>>> +#endif
>>> +        .of_match_table = 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");
>>>
>>
> 
> 

Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver

[Paul Cercueil]

Hi Suman,

Le jeu. 11 juin 2020 à 19:21, Suman Anna <s-anna@ti.com> a écrit :
> Hi Paul,
> 
> On 6/11/20 5:21 PM, Paul Cercueil wrote:
>> Hi Suman,
>> 
>> Le jeu. 11 juin 2020 à 16:47, Suman Anna <s-anna@ti.com> a écrit :
>>> Hi Paul,
>>> 
>>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>>> 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.
>>> 
>>> I have a few comments w.r.t pm_runtime usage in this driver.
>>> 
>>>> 
>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>> 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()
>>>>      v5: No change
>>>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>>>> \x7f\x7f\x7fcallbacks
>>>>      v7: - Remove use of of_match_ptr()
>>>>          - Move Kconfig symbol so that it's in alphabetical order
>>>>          - Add missing doc for private structure field aux_base
>>>>          - Don't check for (len <= 0) in da_to_va()
>>>>          - Add missing \n in dev_info/dev_err messages
>>>> 
>>>>   drivers/remoteproc/Kconfig         |   9 +
>>>>   drivers/remoteproc/Makefile        |   1 +
>>>>   drivers/remoteproc/ingenic_rproc.c | 280 
>>>> +++++++++++++++++++++++++++++
>>>>   3 files changed, 290 insertions(+)
>>>>   create mode 100644 drivers/remoteproc/ingenic_rproc.c
>>>> 
>>>> diff --git a/drivers/remoteproc/Kconfig 
>>>> b/drivers/remoteproc/Kconfig
>>>> index fbaed079b299..c4d1731295eb 100644
>>>> --- a/drivers/remoteproc/Kconfig
>>>> +++ b/drivers/remoteproc/Kconfig
>>>> @@ -23,6 +23,15 @@ config IMX_REMOTEPROC
>>>>   \x7f        It's safe to say N here.
>>>>   \x7f+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 
>>>> \x7f\x7f\x7fIngenic.
>>>> +
>>>> +      This can be either built-in or a loadable module.
>>>> +      If unsure say N.
>>>> +
>>>>   config MTK_SCP
>>>>       tristate "Mediatek SCP support"
>>>>       depends on ARCH_MEDIATEK
>>>> diff --git a/drivers/remoteproc/Makefile 
>>>> b/drivers/remoteproc/Makefile
>>>> index 0effd3825035..e8b886e511f0 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_MTK_SCP)            += mtk_scp.o mtk_scp_ipi.o
>>>>   obj-$(CONFIG_OMAP_REMOTEPROC)        += omap_remoteproc.o
>>>>   obj-$(CONFIG_WKUP_M3_RPROC)        += wkup_m3_rproc.o
>>>> diff --git a/drivers/remoteproc/ingenic_rproc.c 
>>>> \x7f\x7f\x7fb/drivers/remoteproc/ingenic_rproc.c
>>>> new file mode 100644
>>>> index 000000000000..189020d77b25
>>>> --- /dev/null
>>>> +++ b/drivers/remoteproc/ingenic_rproc.c
>>>> @@ -0,0 +1,280 @@
>>>> +// 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/pm_runtime.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
>>>> + * @aux_base: raw pointer to the AUX interface registers
>>>> + * @mem_info: array of struct vpu_mem_info, which contain the 
>>>> \x7f\x7f\x7fmapping 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_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, 
>>>> \x7f\x7f\x7fsize_t len)
>>>> +{
>>>> +    struct vpu *vpu = rproc->priv;
>>>> +    void __iomem *va = NULL;
>>>> +    unsigned int i;
>>>> +
>>>> +    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 = {
>>>> +    .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 void ingenic_rproc_disable_clks(void *data)
>>>> +{
>>>> +    struct vpu *vpu = data;
>>>> +
>>>> +    pm_runtime_resume(vpu->dev);
>>>> +    pm_runtime_disable(vpu->dev);
>>>> +
>>>> +    clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
>>>> +}
>>>> +
>>>> +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;
>>>> +    platform_set_drvdata(pdev, vpu);
>>>> +
>>>> +    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\n");
>>>> +        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\n");
>>>> +            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\n");
>>>> +        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", \x7f\x7f\x7frproc);
>>>> +    if (ret < 0) {
>>>> +        dev_err(dev, "Failed to request IRQ\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    disable_irq(vpu->irq);
>>>> +
>>>> +    /* The clocks must be enabled for the firmware to be loaded 
>>>> in \x7f\x7f\x7fTCSM */
>>>> +    ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), 
>>>> vpu->clks);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "Unable to start clocks\n");
>>>> +        return ret;
>>>> +    }
>>> 
>>> You are enabling the clocks directly here and also trying to manage 
>>> \x7f\x7fthem through pm_runtime callbacks again.
>> 
>> Yes. The clocks need to be enabled in the probe.
> 
> For the preferred non CONFIG_PM case now and lack of 
> prepare/unprepare().

I want to make it clear that I'm not against having 
.prepare/.unprepare, but I want to see what maintainers have to say.

>> 
>>>> +
>>>> +    pm_runtime_irq_safe(dev);
>>> 
>>> Nothing wrong with this, but this does take an additional reference 
>>> \x7f\x7fcount on the parent device (a bus device for you??), and also 
>>> implies \x7f\x7fthat your clk driver code can all run in atomic context 
>>> so unless you \x7f\x7fhave a strong reason, it is safe to drop this.
>> 
>> The clock driver code can run in atomic context, it is guaranteed by 
>> the \x7fclk API.
> 
> OK.
> 
>> 
>>> 
>>>> +    pm_runtime_set_active(dev);
>>> 
>>> The get_sync below would have been sufficient if you had either 
>>> \x7f\x7flimited the clk API above to just clk_prepare, or you could have 
>>> moved \x7f\x7fthe whole clk API above into your runtime resume callback.
>> 
>> You assume that pm_runtime_get() will enable the clocks, but that's 
>> only \x7ftrue if CONFIG_PM is set.
>> 
>> The reason the clocks are enabled in the probe, and 
>> \x7fpm_runtime_set_active() is called, is that it works whether or not 
>> \x7fCONFIG_PM is set.
> 
> As I said, if the intention is to reflect the clocks active state in 
> rpm status, then pm_runtime_get_noresume() does the job for you 
> instead of get_sync(). pm_runtime_get_sync() typically does 3 things 
> - increment the usage count, invoke your callbacks (enable clocks for 
> you), and sets the status to active, with the last two steps optional 
> depending on usage count.

So pm_runtime_get_noresume() instead of pm_runtime_set_active() + 
pm_runtime_get_sync()?

>> 
>>>> +    pm_runtime_enable(dev);
>>>> +    pm_runtime_get_sync(dev);
>>> 
>>> If the intention was to increment the usage count with the above 
>>> \x7f\x7fsequence, pm_runtime_get_noresume() is better. But dropping all 
>>> of the \x7f\x7fabove and just using get_sync would have been sufficient.
>>> 
>>>> +    pm_runtime_use_autosuspend(dev);
>>> 
>>> I don't see any setting of the autosuspend delay (default value is 
>>> 0). \x7f\x7fSo, you might have as well just not used this at all, and 
>>> just used \x7f\x7fpm_runtime_put() below.
>> 
>> Autosuspend delay value can be set from userspace.
> 
> Yes, but I don't see a specific purpose for it in your driver. Either 
> you have remoteproc running (and so clocks enabled always), or you 
> don't have a firmware loaded and want clocks disabled (not sure you 
> would want to waste power for certain of amount of time).

What I had in mind is that it would prevent the hardware from being 
quickly disabled then re-enabled when switching firmwares. But it's not 
like it takes a long time to stop/start the clock, so this could go 
away, yes.

>> 
>>>> +
>>>> +    ret = devm_add_action_or_reset(dev, 
>>>> ingenic_rproc_disable_clks, \x7f\x7f\x7fvpu);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "Unable to register action\n");
>>>> +        goto out_pm_put;
>>>> +    }
>>>> +
>>>> +    ret = devm_rproc_add(dev, rproc);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "Failed to register remote processor\n");
>>>> +        goto out_pm_put;
>>>> +    }
>>> 
>>> You are using auto-boot, so the firmware loading is an asynchronous 
>>> \x7f\x7fevent and most probably you would run through below sequence 
>>> first, \x7f\x7fand end up disabling the clocks with an incorrect rpm 
>>> status.
>> 
>> The driver can auto-load the firmware, but that does not mean it 
>> will. \x7fWe actually don't do that, and load a task-specific firmware 
>> onto the \x7fremote processor on demand.
> 
> Yeah OK, depends on what is preferred by default. If it is more 
> standard practise that the remoteproc is booted by userspace always, 
> then I suggest setting auto_boot as false. But nothing wrong with 
> expecting to boot by default with a starting firmware.

Ok, I didn't know there was a way to disable auto-boot.

>> 
>>>> +
>>>> +out_pm_put:
>>>> +    pm_runtime_put_autosuspend(dev);
>>> 
>>> And finally, with the remoteproc core rpm patch, this would all 
>>> have \x7f\x7fbeen unnecessary.
>>> 
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static const struct of_device_id ingenic_rproc_of_matches[] = {
>>>> +    { .compatible = "ingenic,jz4770-vpu-rproc", },
>>>> +    {}
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
>>>> +
>>>> +static int __maybe_unused ingenic_rproc_suspend(struct device 
>>>> *dev)
>>>> +{
>>>> +    struct vpu *vpu = dev_get_drvdata(dev);
>>>> +
>>>> +    clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused ingenic_rproc_resume(struct device *dev)
>>>> +{
>>>> +    struct vpu *vpu = dev_get_drvdata(dev);
>>>> +
>>>> +    return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
>>>> +    SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, 
>>>> ingenic_rproc_resume, \x7f\x7f\x7fNULL)
>>>> +};
>>>> +
>>>> +static struct platform_driver ingenic_rproc_driver = {
>>>> +    .probe = ingenic_rproc_probe,
>>>> +    .driver = {
>>>> +        .name = "ingenic-vpu",
>>>> +#ifdef CONFIG_PM
>>> 
>>> Not sure why you would want to maintain this conditional, because 
>>> \x7f\x7fruntime_pm is a core dependency now for your driver to work 
>>> properly.
>> 
>> No, it is not - the driver works perfectly fine with CONFIG_PM being 
>> \x7fdisabled, and having a hardcoded dependency on CONFIG_PM is not 
>> \x7fsomething we want.
> 
> OK, so if IIUC, in general CONFIG_PM is not a typical usage for your 
> MIPS platforms. Your driver is the first non-ARM remoteproc driver 
> :), CONFIG_PM is almost a given on most ARM platforms.
> 
> So, I fail to see how your clocks can stay disabled then when 
> CONFIG_PM=n if the remoteproc fails to load with the current code, 
> which was your primary argument for using prepare/unprepare (based on 
> comments on prior versions). It looks to me that your needs are 
> indeed better suited with the prepare/unprepare ops as in your 
> initial series.

Yes, I find the rpm in remoteproc core solution more elegant, but that 
doesn't work as well for me in the CONFIG_PM=n case.

> And in the case of CONFIG_PM=y, you have three levels of code that 
> enables the clocks (the bare clk API, the pm_runtime_get in probe, 
> and the pm_runtime_get in the remoteproc core). Depending on the rpm 
> status, it may or may not invoke the callbacks.

pm_runtime_get() in the probe won't call pm_resume, since the rpm 
status is marked as active with pm_runtime_set_active(), so the clocks 
are enabled only once in the probe. And even if the remoteproc core 
calls pm_runtime_get() in the middle of the probe (why would it?) it 
would still work fine since the clock enable/disable API is 
reference-counted.

Cheers,
-Paul

> regards
> Suman
> 
>> 
>> Cheers,
>> -Paul
>> 
>>> regards
>>> Suman
>>> 
>>>> +        .pm = &ingenic_rproc_pm,
>>>> +#endif
>>>> +        .of_match_table = 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");
>>>> 
>>> 
>> 
>> 
> 

Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver

[Suman Anna]

Hi Paul,

On 6/12/20 6:47 AM, Paul Cercueil wrote:
> Hi Suman,
> 
> Le jeu. 11 juin 2020 à 19:21, Suman Anna <s-anna@ti.com> a écrit :
>> Hi Paul,
>>
>> On 6/11/20 5:21 PM, Paul Cercueil wrote:
>>> Hi Suman,
>>>
>>> Le jeu. 11 juin 2020 à 16:47, Suman Anna <s-anna@ti.com> a écrit :
>>>> Hi Paul,
>>>>
>>>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>>>> 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.
>>>>
>>>> I have a few comments w.r.t pm_runtime usage in this driver.
>>>>
>>>>>
>>>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>> 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()
>>>>>      v5: No change
>>>>>      v6: Instead of prepare/unprepare callbacks, use PM runtime 
>>>>> \x7f\x7f\x7fcallbacks
>>>>>      v7: - Remove use of of_match_ptr()
>>>>>          - Move Kconfig symbol so that it's in alphabetical order
>>>>>          - Add missing doc for private structure field aux_base
>>>>>          - Don't check for (len <= 0) in da_to_va()
>>>>>          - Add missing \n in dev_info/dev_err messages
>>>>>
>>>>>   drivers/remoteproc/Kconfig         |   9 +
>>>>>   drivers/remoteproc/Makefile        |   1 +
>>>>>   drivers/remoteproc/ingenic_rproc.c | 280 
>>>>> +++++++++++++++++++++++++++++
>>>>>   3 files changed, 290 insertions(+)
>>>>>   create mode 100644 drivers/remoteproc/ingenic_rproc.c
>>>>>
>>>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>>>> index fbaed079b299..c4d1731295eb 100644
>>>>> --- a/drivers/remoteproc/Kconfig
>>>>> +++ b/drivers/remoteproc/Kconfig
>>>>> @@ -23,6 +23,15 @@ config IMX_REMOTEPROC
>>>>>   \x7f        It's safe to say N here.
>>>>>   \x7f+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 \x7f\x7f\x7fIngenic.
>>>>> +
>>>>> +      This can be either built-in or a loadable module.
>>>>> +      If unsure say N.
>>>>> +
>>>>>   config MTK_SCP
>>>>>       tristate "Mediatek SCP support"
>>>>>       depends on ARCH_MEDIATEK
>>>>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>>>>> index 0effd3825035..e8b886e511f0 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_MTK_SCP)            += mtk_scp.o mtk_scp_ipi.o
>>>>>   obj-$(CONFIG_OMAP_REMOTEPROC)        += omap_remoteproc.o
>>>>>   obj-$(CONFIG_WKUP_M3_RPROC)        += wkup_m3_rproc.o
>>>>> diff --git a/drivers/remoteproc/ingenic_rproc.c 
>>>>> \x7f\x7f\x7fb/drivers/remoteproc/ingenic_rproc.c
>>>>> new file mode 100644
>>>>> index 000000000000..189020d77b25
>>>>> --- /dev/null
>>>>> +++ b/drivers/remoteproc/ingenic_rproc.c
>>>>> @@ -0,0 +1,280 @@
>>>>> +// 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/pm_runtime.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
>>>>> + * @aux_base: raw pointer to the AUX interface registers
>>>>> + * @mem_info: array of struct vpu_mem_info, which contain the 
>>>>> \x7f\x7f\x7fmapping 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_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, \x7f\x7f\x7fsize_t len)
>>>>> +{
>>>>> +    struct vpu *vpu = rproc->priv;
>>>>> +    void __iomem *va = NULL;
>>>>> +    unsigned int i;
>>>>> +
>>>>> +    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 = {
>>>>> +    .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 void ingenic_rproc_disable_clks(void *data)
>>>>> +{
>>>>> +    struct vpu *vpu = data;
>>>>> +
>>>>> +    pm_runtime_resume(vpu->dev);
>>>>> +    pm_runtime_disable(vpu->dev);
>>>>> +
>>>>> +    clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
>>>>> +}
>>>>> +
>>>>> +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;
>>>>> +    platform_set_drvdata(pdev, vpu);
>>>>> +
>>>>> +    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\n");
>>>>> +        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\n");
>>>>> +            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\n");
>>>>> +        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", \x7f\x7f\x7frproc);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(dev, "Failed to request IRQ\n");
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    disable_irq(vpu->irq);
>>>>> +
>>>>> +    /* The clocks must be enabled for the firmware to be 
>>>>> loaded in \x7f\x7f\x7fTCSM */
>>>>> +    ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
>>>>> +    if (ret) {
>>>>> +        dev_err(dev, "Unable to start clocks\n");
>>>>> +        return ret;
>>>>> +    }
>>>>
>>>> You are enabling the clocks directly here and also trying to 
>>>> manage \x7f\x7fthem through pm_runtime callbacks again.
>>>
>>> Yes. The clocks need to be enabled in the probe.
>>
>> For the preferred non CONFIG_PM case now and lack of prepare/unprepare().
> 
> I want to make it clear that I'm not against having .prepare/.unprepare, 
> but I want to see what maintainers have to say.

Yes, it is the solution though to meet all your needs if CONFIG_PM is 
not used in general.

> 
>>>
>>>>> +
>>>>> +    pm_runtime_irq_safe(dev);
>>>>
>>>> Nothing wrong with this, but this does take an additional 
>>>> reference \x7f\x7fcount on the parent device (a bus device for you??), and 
>>>> also implies \x7f\x7fthat your clk driver code can all run in atomic 
>>>> context so unless you \x7f\x7fhave a strong reason, it is safe to drop this.
>>>
>>> The clock driver code can run in atomic context, it is guaranteed 
>>> by the \x7fclk API.
>>
>> OK.
>>
>>>
>>>>
>>>>> +    pm_runtime_set_active(dev);
>>>>
>>>> The get_sync below would have been sufficient if you had either 
>>>> \x7f\x7flimited the clk API above to just clk_prepare, or you could have 
>>>> moved \x7f\x7fthe whole clk API above into your runtime resume callback.
>>>
>>> You assume that pm_runtime_get() will enable the clocks, but that's 
>>> only \x7ftrue if CONFIG_PM is set.
>>>
>>> The reason the clocks are enabled in the probe, and 
>>> \x7fpm_runtime_set_active() is called, is that it works whether or not 
>>> \x7fCONFIG_PM is set.
>>
>> As I said, if the intention is to reflect the clocks active state in 
>> rpm status, then pm_runtime_get_noresume() does the job for you 
>> instead of get_sync(). pm_runtime_get_sync() typically does 3 things - 
>> increment the usage count, invoke your callbacks (enable clocks for 
>> you), and sets the status to active, with the last two steps optional 
>> depending on usage count.
> 
> So pm_runtime_get_noresume() instead of pm_runtime_set_active() + 
> pm_runtime_get_sync()?

Its, pm_runtime_set_active() + pm_runtime_get_noresume().

They do exactly what you desire in the sequence, clocks are already 
enabled, so you set the status and increment the rpm usage count.

> 
>>>
>>>>> +    pm_runtime_enable(dev);
>>>>> +    pm_runtime_get_sync(dev);
>>>>
>>>> If the intention was to increment the usage count with the above 
>>>> \x7f\x7fsequence, pm_runtime_get_noresume() is better. But dropping all of 
>>>> the \x7f\x7fabove and just using get_sync would have been sufficient.
>>>>
>>>>> +    pm_runtime_use_autosuspend(dev);
>>>>
>>>> I don't see any setting of the autosuspend delay (default value 
>>>> is 0). \x7f\x7fSo, you might have as well just not used this at all, and 
>>>> just used \x7f\x7fpm_runtime_put() below.
>>>
>>> Autosuspend delay value can be set from userspace.
>>
>> Yes, but I don't see a specific purpose for it in your driver. Either 
>> you have remoteproc running (and so clocks enabled always), or you 
>> don't have a firmware loaded and want clocks disabled (not sure you 
>> would want to waste power for certain of amount of time).
> 
> What I had in mind is that it would prevent the hardware from being 
> quickly disabled then re-enabled when switching firmwares. But it's not 
> like it takes a long time to stop/start the clock, so this could go 
> away, yes.

Also note that pm_runtime_put() is already asynchronous, so it doesn't 
happen right away.

> 
>>>
>>>>> +
>>>>> +    ret = devm_add_action_or_reset(dev, 
>>>>> ingenic_rproc_disable_clks, \x7f\x7f\x7fvpu);
>>>>> +    if (ret) {
>>>>> +        dev_err(dev, "Unable to register action\n");
>>>>> +        goto out_pm_put;
>>>>> +    }
>>>>> +
>>>>> +    ret = devm_rproc_add(dev, rproc);
>>>>> +    if (ret) {
>>>>> +        dev_err(dev, "Failed to register remote processor\n");
>>>>> +        goto out_pm_put;
>>>>> +    }
>>>>
>>>> You are using auto-boot, so the firmware loading is an 
>>>> asynchronous \x7f\x7fevent and most probably you would run through below 
>>>> sequence first, \x7f\x7fand end up disabling the clocks with an incorrect 
>>>> rpm status.
>>>
>>> The driver can auto-load the firmware, but that does not mean it 
>>> will. \x7fWe actually don't do that, and load a task-specific firmware 
>>> onto the \x7fremote processor on demand.
>>
>> Yeah OK, depends on what is preferred by default. If it is more 
>> standard practise that the remoteproc is booted by userspace always, 
>> then I suggest setting auto_boot as false. But nothing wrong with 
>> expecting to boot by default with a starting firmware.
> 
> Ok, I didn't know there was a way to disable auto-boot.

Yes, you can just set the rproc->auto_boot = false after rproc_alloc() 
and before rproc_add(). Again, it depends on what you desire. I have 
seen some rproc drivers set this actually based on a DT flag.

> 
>>>
>>>>> +
>>>>> +out_pm_put:
>>>>> +    pm_runtime_put_autosuspend(dev);
>>>>
>>>> And finally, with the remoteproc core rpm patch, this would all 
>>>> have \x7f\x7fbeen unnecessary.
>>>>
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static const struct of_device_id ingenic_rproc_of_matches[] = {
>>>>> +    { .compatible = "ingenic,jz4770-vpu-rproc", },
>>>>> +    {}
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
>>>>> +
>>>>> +static int __maybe_unused ingenic_rproc_suspend(struct device *dev)
>>>>> +{
>>>>> +    struct vpu *vpu = dev_get_drvdata(dev);
>>>>> +
>>>>> +    clk_bulk_disable(ARRAY_SIZE(vpu->clks), vpu->clks);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int __maybe_unused ingenic_rproc_resume(struct device *dev)
>>>>> +{
>>>>> +    struct vpu *vpu = dev_get_drvdata(dev);
>>>>> +
>>>>> +    return clk_bulk_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
>>>>> +}
>>>>> +
>>>>> +static const struct dev_pm_ops __maybe_unused ingenic_rproc_pm = {
>>>>> +    SET_RUNTIME_PM_OPS(ingenic_rproc_suspend, 
>>>>> ingenic_rproc_resume, \x7f\x7f\x7fNULL)
>>>>> +};
>>>>> +
>>>>> +static struct platform_driver ingenic_rproc_driver = {
>>>>> +    .probe = ingenic_rproc_probe,
>>>>> +    .driver = {
>>>>> +        .name = "ingenic-vpu",
>>>>> +#ifdef CONFIG_PM
>>>>
>>>> Not sure why you would want to maintain this conditional, because 
>>>> \x7f\x7fruntime_pm is a core dependency now for your driver to work properly.
>>>
>>> No, it is not - the driver works perfectly fine with CONFIG_PM 
>>> being \x7fdisabled, and having a hardcoded dependency on CONFIG_PM is 
>>> not \x7fsomething we want.
>>
>> OK, so if IIUC, in general CONFIG_PM is not a typical usage for your 
>> MIPS platforms. Your driver is the first non-ARM remoteproc driver :), 
>> CONFIG_PM is almost a given on most ARM platforms.
>>
>> So, I fail to see how your clocks can stay disabled then when 
>> CONFIG_PM=n if the remoteproc fails to load with the current code, 
>> which was your primary argument for using prepare/unprepare (based on 
>> comments on prior versions). It looks to me that your needs are indeed 
>> better suited with the prepare/unprepare ops as in your initial series.
> 
> Yes, I find the rpm in remoteproc core solution more elegant, but that 
> doesn't work as well for me in the CONFIG_PM=n case.
> 
>> And in the case of CONFIG_PM=y, you have three levels of code that 
>> enables the clocks (the bare clk API, the pm_runtime_get in probe, and 
>> the pm_runtime_get in the remoteproc core). Depending on the rpm 
>> status, it may or may not invoke the callbacks.
> 
> pm_runtime_get() in the probe won't call pm_resume, since the rpm status 
> is marked as active with pm_runtime_set_active(), so the clocks are 
> enabled only once in the probe. 

Right, that's what I meant by last sentence above.

And even if the remoteproc core calls
> pm_runtime_get() in the middle of the probe (why would it?) it would 
> still work fine since the clock enable/disable API is reference-counted.

Yeah, mostly it won't happen because of the firmware_class loading, and 
you will run through the end of your probe before you get loaded.

So, how critical is this disabling clocks feature, because it doesn't 
work in your preferred CONFIG_PM=n case. And if the most common way for 
you is to load and unload from userspace anyway, you can just manage the 
clocks in probe and remove, and rely on modprobe or sysfs bind/unbind to 
ensure the clocks stay disabled.

regards
Suman

> 
> Cheers,
> -Paul
> 
>> regards
>> Suman
>>
>>>
>>> Cheers,
>>> -Paul
>>>
>>>> regards
>>>> Suman
>>>>
>>>>> +        .pm = &ingenic_rproc_pm,
>>>>> +#endif
>>>>> +        .of_match_table = 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");
>>>>>
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver

[Bjorn Andersson]

On Fri 12 Jun 04:47 PDT 2020, Paul Cercueil wrote:
> Le jeu. 11 juin 2020 à 19:21, Suman Anna <s-anna@ti.com> a écrit :
> > On 6/11/20 5:21 PM, Paul Cercueil wrote:
> > > Le jeu. 11 juin 2020 à 16:47, Suman Anna <s-anna@ti.com> a écrit :
> > > > On 5/15/20 5:43 AM, Paul Cercueil wrote:
[..]
> > > > > diff --git a/drivers/remoteproc/ingenic_rproc.c
> > > > > \x7f\x7f\x7fb/drivers/remoteproc/ingenic_rproc.c
[..]
> > > > > +    /* The clocks must be enabled for the firmware to be
> > > > > loaded in \x7f\x7f\x7fTCSM */
> > > > > +    ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks),
> > > > > vpu->clks);
> > > > > +    if (ret) {
> > > > > +        dev_err(dev, "Unable to start clocks\n");
> > > > > +        return ret;
> > > > > +    }
> > > > 
> > > > You are enabling the clocks directly here and also trying to
> > > > manage \x7f\x7fthem through pm_runtime callbacks again.
> > > 
> > > Yes. The clocks need to be enabled in the probe.
> > 
> > For the preferred non CONFIG_PM case now and lack of
> > prepare/unprepare().
> 
> I want to make it clear that I'm not against having .prepare/.unprepare, but
> I want to see what maintainers have to say.
> 

I think it's perfectly reasonable to enable all the resources here and
then if CONFIG_PM isn't set you just leave them enabled throughout.

Regards,
Bjorn

Re: [PATCH v7 3/5] remoteproc: Add support for runtime PM

[Arnaud POULIQUEN]

Hi, 

On 6/11/20 11:17 PM, Suman Anna wrote:
> On 6/10/20 11:39 PM, Bjorn Andersson wrote:
>> On Wed 10 Jun 02:40 PDT 2020, Paul Cercueil wrote:
>>
>>> Hi,
>>>
>>> Le lun. 8 juin 2020 à 18:10, Suman Anna <s-anna@ti.com> a écrit :
>>>> Hi Paul,
>>>>
>>>> On 6/8/20 5:46 PM, Paul Cercueil wrote:
>>>>> Hi Suman,
>>>>>
>>>>>>>> On 5/15/20 5:43 AM, Paul Cercueil wrote:
>>>>>>>>> Call pm_runtime_get_sync() before the firmware is loaded, and
>>>>>>>>> pm_runtime_put() after the remote processor has been stopped.
>>>>>>>>>
>>>>>>>>> Even though the remoteproc device has no PM
>>>>>>>>> callbacks, this allows the
>>>>>>>>> parent device's PM callbacks to be properly called.
>>>>>>>>
>>>>>>>> I see this patch staged now for 5.8, and the latest
>>>>>>>> -next branch \x7f\x7f\x7f\x7fhas \x7f\x7fbroken the pm-runtime autosuspend
>>>>>>>> feature we have in the \x7f\x7f\x7f\x7fOMAP \x7f\x7fremoteproc driver. See
>>>>>>>> commit 5f31b232c674 ("remoteproc/omap: \x7f\x7f\x7f\x7fAdd \x7f\x7fsupport
>>>>>>>> for runtime auto-suspend/resume").
>>>>>>>>
>>>>>>>> What was the original purpose of this patch, because
>>>>>>>> there can be \x7f\x7f\x7f\x7f\x7f\x7fdiffering backends across different
>>>>>>>> SoCs.
>>>>>>>
>>>>>>> Did you try pm_suspend_ignore_children()? It looks like it
>>>>>>> was made \x7f\x7f\x7ffor \x7fyour use-case.
>>>>>>
>>>>>> Sorry for the delay in getting back. So, using
>>>>>> \x7f\x7fpm_suspend_ignore_children() does fix my current issue.
>>>>>>
>>>>>> But I still fail to see the original purpose of this patch in
>>>>>> the \x7f\x7fremoteproc core especially given that the core itself does
>>>>>> not have \x7f\x7fany callbacks. If the sole intention was to call the
>>>>>> parent pdev's \x7f\x7fcallbacks, then I feel that state-machine is
>>>>>> better managed within \x7f\x7fthat particular platform driver itself,
>>>>>> as the sequencing/device \x7f\x7fmanagement can vary with different
>>>>>> platform drivers.
>>>>>
>>>>> The problem is that with Ingenic SoCs some clocks must be enabled in
>>>>> \x7forder to load the firmware, and the core doesn't give you an option
>>>>> to \x7fregister a callback to be called before loading it.
>>>>
>>>> Yep, I have similar usage in one of my remoteproc drivers (see
>>>> keystone_remoteproc.c), and I think this all stems from the need to
>>>> use/support loading into a processor's internal memories. My driver does
>>>> leverage the pm-clks backend plugged into pm_runtime, so you won't see
>>>> explicit calls on the clocks.
>>>>
>>>> I guess the question is what exact PM features you are looking for with
>>>> the Ingenic SoC. I do see you are using pm_runtime autosuspend, and your
>>>> callbacks are managing the clocks, but reset is managed only in
>>>> start/stop.
>>>>
>>>>> The first version of \x7fmy patchset added .prepare/.unprepare
>>>>> callbacks to the struct rproc_ops, \x7fbut the feedback from the
>>>>> maintainers was that I should do it via \x7fruntime PM. However, it was
>>>>> not possible to keep it contained in the \x7fdriver, since again the
>>>>> core doesn't provide a "prepare" callback, so no \x7fplace to call
>>>>> pm_runtime_get_sync().
>>>> FWIW, the .prepare/.unprepare callbacks is actually now part of the
>>>> rproc core. Looks like multiple developers had a need for this, and this
>>>> functionality went in at the same time as your driver :). Not sure if
>>>> you looked up the prior patches, I leveraged the patch that Loic had
>>>> submitted a long-time ago, and a revised version of it is now part of
>>>> 5.8-rc1.
>>>
>>> WTF maintainers, you refuse my patchset for adding a .prepare/.unprepare,
>>> ask me to do it via runtime PM, then merge another patchset that adds these
>>> callback. At least be constant in your decisions.
>>>
>>
>> Sorry, I missed this when applying the two patches, but you're of course
>> right.
>>
>>> Anyway, now we have two methods added to linux-next for doing the exact same
>>> thing. What should we do about it?
>>>
>>
>> I like the pm_runtime approach and as it was Arnaud that asked you to
>> change it, perhaps he and Loic can agree on updating the ST driver so we
>> can drop the prepare/unprepare ops again?
> 
> These callbacks were added primarily in preparation for the TI K3 rproc 
> drivers, not just ST (the patch was resurrected from a very old patch 
> from Loic).
> 
> I still think prepare/unprepare is actually better suited to scale well 
> for the long term. This pm_runtime logic will now make the early-boot 
> scenarios complicated, as you would have to match its status, but all 
> actual operations are on the actual parent remoteproc platform device 
> and not the child remoteproc device. I think it serves to mess up the 
> state-machines of different platform drivers due to additional refcounts 
> acquired and maybe performing some operations out of sequence to what a 
> platform driver wants esp. if there is automated backend usage like 
> genpd, pm_clks etc. I am yet to review Mathieu's latest MCU sync series, 
> but the concept of different sync_ops already scales w.r.t the 
> prepare/unprepare.


> 
> As for my K3 drivers, the callbacks are doing more than just turning on 
> clocks, as the R5Fs in general as a complex power-on sequence. I do not 
> have remoteproc auto-suspend atm on the K3 drivers, but that typically 
> means shutting down and restoring the core and would involve all the 
> hardware-specific sequences, so the rpm callback implementations will be 
> more than just clocks.
> 
> I looked through the patch history on the Ingenic remoteproc driver, and 
> the only reason for either of runtime pm usage or prepare/unprepare ops 
> usage is to ensure that clocks do not stay enabled in the case the 
> processor is not loaded/started. The driver is using auto-boot, so when 
> it probes, in general we expect the remoteproc to be running. So, the 
> only failure case is if there is no firmware. Otherwise, Paul could have 
> just used clk_bulk API in probe and remove.
> 
> Anyway, I will provide some additional review comments on the pm_runtime 
> usage within the Ingenic rproc driver.

Sorry for the late answer...

Prepare/unprepare was proposed by Loïc for the memory management series.
We abandoned it and migrated the memory registrations in parse_fw ops.
So we don't use it anymore in ST.

I suggested the pm-runtime before TI patchset, as it was matching
with Ingenic driver need. 
Now with the additional need introduced by Suman, seems that it is not sufficient
for the K3-r5, due to the memory initialization.

Anyway do we have to choice between the 2 implementations?
Look to me that the pm_runtime could be used to manage the power for the
remoteproc device (and perhaps associated power domain).
While the .prepare is more for additional resources
which ,need to be initialized before loading the firmware (such as some specific
system resources).

Regards,
Arnaud

> 
> regards
> Suman
> 

Re: [PATCH v7 4/5] remoteproc: ingenic: Added remoteproc driver

[Mathieu Poirier]

On Sun, Jun 21, 2020 at 12:30:22PM -0700, Bjorn Andersson wrote:
> On Fri 12 Jun 04:47 PDT 2020, Paul Cercueil wrote:
> > Le jeu. 11 juin 2020 à 19:21, Suman Anna <s-anna@ti.com> a écrit :
> > > On 6/11/20 5:21 PM, Paul Cercueil wrote:
> > > > Le jeu. 11 juin 2020 à 16:47, Suman Anna <s-anna@ti.com> a écrit :
> > > > > On 5/15/20 5:43 AM, Paul Cercueil wrote:
> [..]
> > > > > > diff --git a/drivers/remoteproc/ingenic_rproc.c
> > > > > > \x7f\x7f\x7fb/drivers/remoteproc/ingenic_rproc.c
> [..]
> > > > > > +    /* The clocks must be enabled for the firmware to be
> > > > > > loaded in \x7f\x7f\x7fTCSM */
> > > > > > +    ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks),
> > > > > > vpu->clks);
> > > > > > +    if (ret) {
> > > > > > +        dev_err(dev, "Unable to start clocks\n");
> > > > > > +        return ret;
> > > > > > +    }
> > > > > 
> > > > > You are enabling the clocks directly here and also trying to
> > > > > manage \x7f\x7fthem through pm_runtime callbacks again.
> > > > 
> > > > Yes. The clocks need to be enabled in the probe.
> > > 
> > > For the preferred non CONFIG_PM case now and lack of
> > > prepare/unprepare().
> > 
> > I want to make it clear that I'm not against having .prepare/.unprepare, but
> > I want to see what maintainers have to say.
> > 
> 
> I think it's perfectly reasonable to enable all the resources here and
> then if CONFIG_PM isn't set you just leave them enabled throughout.

In my opinion the best way to deal with the status of the CONFIG_PM
configuration option is to move clock relate operations to the prepare/unprepare
callbacks.  Doing so would have several advantages:

1) If rproc->auto_boot is false then clocks aren't enabled needlessly between
the time the driver is probed and the remote processor is started.

2) It would allow for the removal of the runtime PM calls in the core, which in
the current state, prevents the runtime PM mechanic to really serve its purpose.
Indeed, calling runtime PM operations in rproc_fw_boot() and rproc_shutdown()
prevents the remote processor from being suspended during periods of inactivity.
If all that is required for Ingenic's remote processor is to set the clocks
before accessing device memory and switch them off when no longer needed, I
think prepare() and unprepare() are the best choice.

3) As Suman pointed out having runtime PM operations in remoteproc core makes the
task of supporting scenarios where the remote processor is loaded by another
entity more complex.  Given the distinct nature of managing power for remote
processors and the characteristics of each platform I beleive the best place to
call runtime PM operations is in the rproc operations, as it is done
omap_remoteproc.c

Since we are quite early in the release cycle all we need is a couple of small
patches: one to move clock manipulation in the Ingenic driver to the remoteproc
core's prepare/unprepare function and another one to remove runtime PM
operations from the core.

Thanks,
Mathieu

> 
> Regards,
> Bjorn