[PATCH v3 1/4] dt-bindings: Document JZ47xx VPU auxiliary processor

[PATCH v3 1/4] dt-bindings: Document JZ47xx VPU auxiliary processor

[Paul Cercueil]

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

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

Notes:
    v2: Update TCSM0 address in example
    v3: Change node name to 'video-decoder'

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

diff --git a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.txt b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.txt
new file mode 100644
index 000000000000..54566e941869
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.txt
@@ -0,0 +1,36 @@
+* Ingenic JZ47xx auxiliary processor
+
+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.
+
+Required properties:
+- compatible: Should be "ingenic,jz4770-vpu-rproc"
+- reg: Must contain the registers location and length for:
+  * the auxiliary processor,
+  * the Tightly Coupled Shared Memory 0 (TCSM0),
+  * the Tightly Coupled Shared Memory 1 (TCSM1),
+  * the shared SRAM.
+- reg-names: Must contain "aux", "tcsm0", "tcsm1", "sram".
+- clocks: Clock specifier for the AUX and VPU clocks.
+- clock-names: Must contain "aux", "vpu".
+- interrupts: Interrupt specifier for the VPU hardware block.
+
+Example:
+
+vpu: video-decoder@132a0000 {
+	compatible = "ingenic,jz4770-vpu-rproc";
+
+	reg = <0x132a0000 0x20 /* AUX */
+		   0x132b0000 0x4000 /* TCSM0 */
+		   0x132c0000 0xc000 /* TCSM1 */
+		   0x132f0000 0x7000 /* SRAM */
+	>;
+	reg-names = "aux", "tcsm0", "tcsm1", "sram";
+
+	clocks = <&cgu JZ4770_CLK_AUX>, <&cgu JZ4770_CLK_VPU>;
+	clock-names = "aux", "vpu";
+
+	interrupt-parent = <&cpuintc>;
+	interrupts = <3>;
+};
-- 
2.24.0

[PATCH v3 2/4] remoteproc: Add device-managed variants of rproc_alloc/rproc_add

[Paul Cercueil]

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

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

Notes:
    v3: New patch

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

[PATCH v3 3/4] remoteproc: Add prepare/unprepare callbacks

[Paul Cercueil]

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

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

Notes:
    v2: No change
    v3: No change

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

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

[PATCH v3 4/4] 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>
---

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

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

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

Re: [PATCH v3 4/4] remoteproc: ingenic: Added remoteproc driver

[Mathieu Poirier]

Hi Paul,

On Sat, Nov 16, 2019 at 06:08:46PM +0100, 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>
> ---
> 
> 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()
> 
>  drivers/remoteproc/Kconfig         |   8 +
>  drivers/remoteproc/Makefile        |   1 +
>  drivers/remoteproc/ingenic_rproc.c | 242 +++++++++++++++++++++++++++++
>  3 files changed, 251 insertions(+)
>  create mode 100644 drivers/remoteproc/ingenic_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 94afdde4bc9f..f7d4b183d152 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -214,6 +214,14 @@ config STM32_RPROC
>  
>  	  This can be either built-in or a loadable module.
>  
> +config INGENIC_RPROC

Are you sure you don't want to make this something like
INGENIC_RPROC_JZ47XX_VPU so that future remote proc system from Ingenic can be
added easily?

> +	tristate "Ingenic JZ47xx VPU remoteproc support"
> +	depends on MIPS || COMPILE_TEST
> +	help
> +	  Say y or m here to support the VPU in the JZ47xx SoCs from Ingenic.
> +	  This can be either built-in or a loadable module.
> +	  If unsure say N.
> +
>  endif # REMOTEPROC
>  
>  endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 00f09e658cb3..6eb0137abbc7 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_RPROC)			+= ingenic_rproc.a

This comes out as one tab to many on my side.

>  obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>  obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
>  obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
> diff --git a/drivers/remoteproc/ingenic_rproc.c b/drivers/remoteproc/ingenic_rproc.c
> new file mode 100644
> index 000000000000..5a7c7d8d9a90
> --- /dev/null
> +++ b/drivers/remoteproc/ingenic_rproc.c
> @@ -0,0 +1,242 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Ingenic JZ47xx remoteproc driver
> + * Copyright 2019, Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +
> +#include "remoteproc_internal.h"
> +
> +#define REG_AUX_CTRL		0x0
> +#define REG_AUX_MSG_ACK		0x10
> +#define REG_AUX_MSG		0x14
> +#define REG_CORE_MSG_ACK	0x18
> +#define REG_CORE_MSG		0x1C
> +
> +#define AUX_CTRL_SLEEP		BIT(31)
> +#define AUX_CTRL_MSG_IRQ_EN	BIT(3)
> +#define AUX_CTRL_NMI_RESETS	BIT(2)
> +#define AUX_CTRL_NMI		BIT(1)
> +#define AUX_CTRL_SW_RESET	BIT(0)
> +
> +struct vpu_mem_map {
> +	const char *name;
> +	unsigned int da;
> +};
> +
> +struct vpu_mem_info {
> +	const struct vpu_mem_map *map;
> +	unsigned long len;
> +	void __iomem *base;
> +};
> +
> +static const struct vpu_mem_map vpu_mem_map[] = {
> +	{ "tcsm0", 0x132b0000 },
> +	{ "tcsm1", 0xf4000000 },
> +	{ "sram",  0x132f0000 },
> +};
> +
> +/* Device data */
> +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;
> +};

Documentation of structure fields is always appreciated but it is up to Bjorn to
decide on this one.

> +
> +static int ingenic_rproc_prepare(struct rproc *rproc)
> +{
> +	struct vpu *vpu = rproc->priv;
> +	int ret;
> +
> +	/* The clocks must be enabled for the firmware to be loaded in TCSM */
> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
> +	if (ret)
> +		dev_err(vpu->dev, "Unable to start clocks: %d", ret);
> +
> +	return ret;
> +}
> +
> +static void ingenic_rproc_unprepare(struct rproc *rproc)
> +{
> +	struct vpu *vpu = rproc->priv;
> +
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
> +}
> +
> +static int ingenic_rproc_start(struct rproc *rproc)
> +{
> +	struct vpu *vpu = rproc->priv;
> +	u32 ctrl;
> +
> +	enable_irq(vpu->irq);
> +
> +	/* Reset the AUX and enable message IRQ */
> +	ctrl = AUX_CTRL_NMI_RESETS | AUX_CTRL_NMI | AUX_CTRL_MSG_IRQ_EN;
> +	writel(ctrl, vpu->aux_base + REG_AUX_CTRL);

Out of curiosity, there is no power domain associated with this
co-processor?  Clocks are all you need?

> +
> +	return 0;
> +}
> +
> +static int ingenic_rproc_stop(struct rproc *rproc)
> +{
> +	struct vpu *vpu = rproc->priv;
> +
> +	/* Keep AUX in reset mode */
> +	writel(AUX_CTRL_SW_RESET, vpu->aux_base + REG_AUX_CTRL);
> +
> +	disable_irq_nosync(vpu->irq);

Also out of curiosity, why the need for _nosync()?

> +
> +	return 0;
> +}
> +
> +static void ingenic_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct vpu *vpu = rproc->priv;
> +
> +	writel(vqid, vpu->aux_base + REG_CORE_MSG);
> +}
> +
> +static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
> +{
> +	struct vpu *vpu = rproc->priv;
> +	void __iomem *va = NULL;
> +	unsigned int i;
> +
> +	if (len <= 0)
> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
> +		const struct vpu_mem_info *info = &vpu->mem_info[i];
> +		const struct vpu_mem_map *map = info->map;
> +
> +		if (da >= map->da && (da + len) < (map->da + info->len)) {
> +			va = info->base + (da - map->da);
> +			break;
> +		}
> +	}
> +
> +	return (__force void *)va;
> +}
> +
> +static struct rproc_ops ingenic_rproc_ops = {
> +	.prepare = ingenic_rproc_prepare,
> +	.unprepare = ingenic_rproc_unprepare,
> +	.start = ingenic_rproc_start,
> +	.stop = ingenic_rproc_stop,
> +	.kick = ingenic_rproc_kick,
> +	.da_to_va = ingenic_rproc_da_to_va,
> +};
> +
> +static irqreturn_t vpu_interrupt(int irq, void *data)
> +{
> +	struct rproc *rproc = data;
> +	struct vpu *vpu = rproc->priv;
> +	u32 vring;
> +
> +	vring = readl(vpu->aux_base + REG_AUX_MSG);
> +
> +	/* Ack the interrupt */
> +	writel(0, vpu->aux_base + REG_AUX_MSG_ACK);
> +
> +	return rproc_vq_interrupt(rproc, vring);
> +}
> +
> +static int ingenic_rproc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *mem;
> +	struct rproc *rproc;
> +	struct vpu *vpu;
> +	unsigned int i;
> +	int ret;
> +
> +	rproc = devm_rproc_alloc(dev, "ingenic-vpu",
> +				 &ingenic_rproc_ops, NULL, sizeof(*vpu));
> +	if (!rproc)
> +		return -ENOMEM;
> +
> +	vpu = rproc->priv;
> +	vpu->dev = &pdev->dev;
> +
> +	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
> +	vpu->aux_base = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(vpu->aux_base)) {
> +		dev_err(dev, "Failed to ioremap");
> +		return PTR_ERR(vpu->aux_base);
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
> +		mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						   vpu_mem_map[i].name);
> +
> +		vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
> +		if (IS_ERR(vpu->mem_info[i].base)) {
> +			ret = PTR_ERR(vpu->mem_info[i].base);
> +			dev_err(dev, "Failed to ioremap");
> +			return ret;
> +		}
> +
> +		vpu->mem_info[i].len = resource_size(mem);
> +		vpu->mem_info[i].map = &vpu_mem_map[i];
> +	}
> +
> +	vpu->clks[0].id = "vpu";
> +	vpu->clks[1].id = "aux";
> +
> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(vpu->clks), vpu->clks);
> +	if (ret) {
> +		dev_err(dev, "Failed to get clocks");
> +		return ret;
> +	}
> +
> +	vpu->irq = platform_get_irq(pdev, 0);
> +	if (vpu->irq < 0) {
> +		dev_err(dev, "Failed to get platform IRQ");
> +		return vpu->irq;
> +	}
> +
> +	ret = devm_request_irq(dev, vpu->irq, vpu_interrupt, 0, "VPU", rproc);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to request IRQ");
> +		return ret;
> +	}
> +
> +	disable_irq_nosync(vpu->irq);
> +
> +	ret = devm_rproc_add(dev, rproc);
> +	if (ret) {
> +		dev_err(dev, "Failed to register remote processor");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ingenic_rproc_of_matches[] = {
> +	{ .compatible = "ingenic,jz4770-vpu-rproc", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
> +
> +static struct platform_driver ingenic_rproc_driver = {
> +	.probe = ingenic_rproc_probe,
> +	.driver = {
> +		.name = "ingenic-vpu",
> +		.of_match_table = of_match_ptr(ingenic_rproc_of_matches),
> +	},
> +};
> +module_platform_driver(ingenic_rproc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control driver");

Nice, clean and simple driver.  With the above and after rolling in Julia's
patch:

Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> -- 
> 2.24.0
> 

Re: [PATCH v3 4/4] remoteproc: ingenic: Added remoteproc driver

[Paul Cercueil]

Hi Mathieu,


Le lun., nov. 18, 2019 at 15:56, Mathieu Poirier 
<mathieu.poirier@linaro.org> a écrit :
> Hi Paul,
> 
> On Sat, Nov 16, 2019 at 06:08:46PM +0100, 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>
>>  ---
>> 
>>  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()
>> 
>>   drivers/remoteproc/Kconfig         |   8 +
>>   drivers/remoteproc/Makefile        |   1 +
>>   drivers/remoteproc/ingenic_rproc.c | 242 
>> +++++++++++++++++++++++++++++
>>   3 files changed, 251 insertions(+)
>>   create mode 100644 drivers/remoteproc/ingenic_rproc.c
>> 
>>  diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>  index 94afdde4bc9f..f7d4b183d152 100644
>>  --- a/drivers/remoteproc/Kconfig
>>  +++ b/drivers/remoteproc/Kconfig
>>  @@ -214,6 +214,14 @@ config STM32_RPROC
>> 
>>   	  This can be either built-in or a loadable module.
>> 
>>  +config INGENIC_RPROC
> 
> Are you sure you don't want to make this something like
> INGENIC_RPROC_JZ47XX_VPU so that future remote proc system from 
> Ingenic can be
> added easily?

Ingenic SoCs aren't named JZ47xx anymore; I'll name the symbol 
INGENIC_VPU_RPROC then.

>>  +	tristate "Ingenic JZ47xx VPU remoteproc support"
>>  +	depends on MIPS || COMPILE_TEST
>>  +	help
>>  +	  Say y or m here to support the VPU in the JZ47xx SoCs from 
>> Ingenic.
>>  +	  This can be either built-in or a loadable module.
>>  +	  If unsure say N.
>>  +
>>   endif # REMOTEPROC
>> 
>>   endmenu
>>  diff --git a/drivers/remoteproc/Makefile 
>> b/drivers/remoteproc/Makefile
>>  index 00f09e658cb3..6eb0137abbc7 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_RPROC)			+= ingenic_rproc.a
> 
> This comes out as one tab to many on my side.
> 
>>   obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>>   obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
>>   obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
>>  diff --git a/drivers/remoteproc/ingenic_rproc.c 
>> b/drivers/remoteproc/ingenic_rproc.c
>>  new file mode 100644
>>  index 000000000000..5a7c7d8d9a90
>>  --- /dev/null
>>  +++ b/drivers/remoteproc/ingenic_rproc.c
>>  @@ -0,0 +1,242 @@
>>  +// SPDX-License-Identifier: GPL-2.0+
>>  +/*
>>  + * Ingenic JZ47xx remoteproc driver
>>  + * Copyright 2019, Paul Cercueil <paul@crapouillou.net>
>>  + */
>>  +
>>  +#include <linux/bitops.h>
>>  +#include <linux/clk.h>
>>  +#include <linux/err.h>
>>  +#include <linux/interrupt.h>
>>  +#include <linux/io.h>
>>  +#include <linux/module.h>
>>  +#include <linux/platform_device.h>
>>  +#include <linux/remoteproc.h>
>>  +
>>  +#include "remoteproc_internal.h"
>>  +
>>  +#define REG_AUX_CTRL		0x0
>>  +#define REG_AUX_MSG_ACK		0x10
>>  +#define REG_AUX_MSG		0x14
>>  +#define REG_CORE_MSG_ACK	0x18
>>  +#define REG_CORE_MSG		0x1C
>>  +
>>  +#define AUX_CTRL_SLEEP		BIT(31)
>>  +#define AUX_CTRL_MSG_IRQ_EN	BIT(3)
>>  +#define AUX_CTRL_NMI_RESETS	BIT(2)
>>  +#define AUX_CTRL_NMI		BIT(1)
>>  +#define AUX_CTRL_SW_RESET	BIT(0)
>>  +
>>  +struct vpu_mem_map {
>>  +	const char *name;
>>  +	unsigned int da;
>>  +};
>>  +
>>  +struct vpu_mem_info {
>>  +	const struct vpu_mem_map *map;
>>  +	unsigned long len;
>>  +	void __iomem *base;
>>  +};
>>  +
>>  +static const struct vpu_mem_map vpu_mem_map[] = {
>>  +	{ "tcsm0", 0x132b0000 },
>>  +	{ "tcsm1", 0xf4000000 },
>>  +	{ "sram",  0x132f0000 },
>>  +};
>>  +
>>  +/* Device data */
>>  +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;
>>  +};
> 
> Documentation of structure fields is always appreciated but it is up 
> to Bjorn to
> decide on this one.

It's a private structure within a driver, does it matter that much?
Besides, it's pretty much self-explanatory.

>>  +
>>  +static int ingenic_rproc_prepare(struct rproc *rproc)
>>  +{
>>  +	struct vpu *vpu = rproc->priv;
>>  +	int ret;
>>  +
>>  +	/* The clocks must be enabled for the firmware to be loaded in 
>> TCSM */
>>  +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
>>  +	if (ret)
>>  +		dev_err(vpu->dev, "Unable to start clocks: %d", ret);
>>  +
>>  +	return ret;
>>  +}
>>  +
>>  +static void ingenic_rproc_unprepare(struct rproc *rproc)
>>  +{
>>  +	struct vpu *vpu = rproc->priv;
>>  +
>>  +	clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
>>  +}
>>  +
>>  +static int ingenic_rproc_start(struct rproc *rproc)
>>  +{
>>  +	struct vpu *vpu = rproc->priv;
>>  +	u32 ctrl;
>>  +
>>  +	enable_irq(vpu->irq);
>>  +
>>  +	/* Reset the AUX and enable message IRQ */
>>  +	ctrl = AUX_CTRL_NMI_RESETS | AUX_CTRL_NMI | AUX_CTRL_MSG_IRQ_EN;
>>  +	writel(ctrl, vpu->aux_base + REG_AUX_CTRL);
> 
> Out of curiosity, there is no power domain associated with this
> co-processor?  Clocks are all you need?

That's correct.

>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static int ingenic_rproc_stop(struct rproc *rproc)
>>  +{
>>  +	struct vpu *vpu = rproc->priv;
>>  +
>>  +	/* Keep AUX in reset mode */
>>  +	writel(AUX_CTRL_SW_RESET, vpu->aux_base + REG_AUX_CTRL);
>>  +
>>  +	disable_irq_nosync(vpu->irq);
> 
> Also out of curiosity, why the need for _nosync()?

It probably doesn't need it. I'll remove it.

>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static void ingenic_rproc_kick(struct rproc *rproc, int vqid)
>>  +{
>>  +	struct vpu *vpu = rproc->priv;
>>  +
>>  +	writel(vqid, vpu->aux_base + REG_CORE_MSG);
>>  +}
>>  +
>>  +static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da, 
>> int len)
>>  +{
>>  +	struct vpu *vpu = rproc->priv;
>>  +	void __iomem *va = NULL;
>>  +	unsigned int i;
>>  +
>>  +	if (len <= 0)
>>  +		return NULL;
>>  +
>>  +	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
>>  +		const struct vpu_mem_info *info = &vpu->mem_info[i];
>>  +		const struct vpu_mem_map *map = info->map;
>>  +
>>  +		if (da >= map->da && (da + len) < (map->da + info->len)) {
>>  +			va = info->base + (da - map->da);
>>  +			break;
>>  +		}
>>  +	}
>>  +
>>  +	return (__force void *)va;
>>  +}
>>  +
>>  +static struct rproc_ops ingenic_rproc_ops = {
>>  +	.prepare = ingenic_rproc_prepare,
>>  +	.unprepare = ingenic_rproc_unprepare,
>>  +	.start = ingenic_rproc_start,
>>  +	.stop = ingenic_rproc_stop,
>>  +	.kick = ingenic_rproc_kick,
>>  +	.da_to_va = ingenic_rproc_da_to_va,
>>  +};
>>  +
>>  +static irqreturn_t vpu_interrupt(int irq, void *data)
>>  +{
>>  +	struct rproc *rproc = data;
>>  +	struct vpu *vpu = rproc->priv;
>>  +	u32 vring;
>>  +
>>  +	vring = readl(vpu->aux_base + REG_AUX_MSG);
>>  +
>>  +	/* Ack the interrupt */
>>  +	writel(0, vpu->aux_base + REG_AUX_MSG_ACK);
>>  +
>>  +	return rproc_vq_interrupt(rproc, vring);
>>  +}
>>  +
>>  +static int ingenic_rproc_probe(struct platform_device *pdev)
>>  +{
>>  +	struct device *dev = &pdev->dev;
>>  +	struct resource *mem;
>>  +	struct rproc *rproc;
>>  +	struct vpu *vpu;
>>  +	unsigned int i;
>>  +	int ret;
>>  +
>>  +	rproc = devm_rproc_alloc(dev, "ingenic-vpu",
>>  +				 &ingenic_rproc_ops, NULL, sizeof(*vpu));
>>  +	if (!rproc)
>>  +		return -ENOMEM;
>>  +
>>  +	vpu = rproc->priv;
>>  +	vpu->dev = &pdev->dev;
>>  +
>>  +	mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
>>  +	vpu->aux_base = devm_ioremap_resource(dev, mem);
>>  +	if (IS_ERR(vpu->aux_base)) {
>>  +		dev_err(dev, "Failed to ioremap");
>>  +		return PTR_ERR(vpu->aux_base);
>>  +	}
>>  +
>>  +	for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
>>  +		mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>  +						   vpu_mem_map[i].name);
>>  +
>>  +		vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
>>  +		if (IS_ERR(vpu->mem_info[i].base)) {
>>  +			ret = PTR_ERR(vpu->mem_info[i].base);
>>  +			dev_err(dev, "Failed to ioremap");
>>  +			return ret;
>>  +		}
>>  +
>>  +		vpu->mem_info[i].len = resource_size(mem);
>>  +		vpu->mem_info[i].map = &vpu_mem_map[i];
>>  +	}
>>  +
>>  +	vpu->clks[0].id = "vpu";
>>  +	vpu->clks[1].id = "aux";
>>  +
>>  +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(vpu->clks), vpu->clks);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to get clocks");
>>  +		return ret;
>>  +	}
>>  +
>>  +	vpu->irq = platform_get_irq(pdev, 0);
>>  +	if (vpu->irq < 0) {
>>  +		dev_err(dev, "Failed to get platform IRQ");
>>  +		return vpu->irq;
>>  +	}
>>  +
>>  +	ret = devm_request_irq(dev, vpu->irq, vpu_interrupt, 0, "VPU", 
>> rproc);
>>  +	if (ret < 0) {
>>  +		dev_err(dev, "Failed to request IRQ");
>>  +		return ret;
>>  +	}
>>  +
>>  +	disable_irq_nosync(vpu->irq);
>>  +
>>  +	ret = devm_rproc_add(dev, rproc);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to register remote processor");
>>  +		return ret;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static const struct of_device_id ingenic_rproc_of_matches[] = {
>>  +	{ .compatible = "ingenic,jz4770-vpu-rproc", },
>>  +	{}
>>  +};
>>  +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
>>  +
>>  +static struct platform_driver ingenic_rproc_driver = {
>>  +	.probe = ingenic_rproc_probe,
>>  +	.driver = {
>>  +		.name = "ingenic-vpu",
>>  +		.of_match_table = of_match_ptr(ingenic_rproc_of_matches),
>>  +	},
>>  +};
>>  +module_platform_driver(ingenic_rproc_driver);
>>  +
>>  +MODULE_LICENSE("GPL");
>>  +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>>  +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control 
>> driver");
> 
> Nice, clean and simple driver.  With the above and after rolling in 
> Julia's
> patch:
> 
> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Thanks!

Cheers,
-Paul

Re: [PATCH v3 4/4] remoteproc: ingenic: Added remoteproc driver

[Mathieu Poirier]

On Fri, 22 Nov 2019 at 11:55, Paul Cercueil <paul@crapouillou.net> wrote:
>
> Hi Mathieu,
>
>
> Le lun., nov. 18, 2019 at 15:56, Mathieu Poirier
> <mathieu.poirier@linaro.org> a écrit :
> > Hi Paul,
> >
> > On Sat, Nov 16, 2019 at 06:08:46PM +0100, 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>
> >>  ---
> >>
> >>  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()
> >>
> >>   drivers/remoteproc/Kconfig         |   8 +
> >>   drivers/remoteproc/Makefile        |   1 +
> >>   drivers/remoteproc/ingenic_rproc.c | 242
> >> +++++++++++++++++++++++++++++
> >>   3 files changed, 251 insertions(+)
> >>   create mode 100644 drivers/remoteproc/ingenic_rproc.c
> >>
> >>  diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >>  index 94afdde4bc9f..f7d4b183d152 100644
> >>  --- a/drivers/remoteproc/Kconfig
> >>  +++ b/drivers/remoteproc/Kconfig
> >>  @@ -214,6 +214,14 @@ config STM32_RPROC
> >>
> >>        This can be either built-in or a loadable module.
> >>
> >>  +config INGENIC_RPROC
> >
> > Are you sure you don't want to make this something like
> > INGENIC_RPROC_JZ47XX_VPU so that future remote proc system from
> > Ingenic can be
> > added easily?
>
> Ingenic SoCs aren't named JZ47xx anymore; I'll name the symbol
> INGENIC_VPU_RPROC then.
>
> >>  +   tristate "Ingenic JZ47xx VPU remoteproc support"
> >>  +   depends on MIPS || COMPILE_TEST
> >>  +   help
> >>  +     Say y or m here to support the VPU in the JZ47xx SoCs from
> >> Ingenic.
> >>  +     This can be either built-in or a loadable module.
> >>  +     If unsure say N.
> >>  +
> >>   endif # REMOTEPROC
> >>
> >>   endmenu
> >>  diff --git a/drivers/remoteproc/Makefile
> >> b/drivers/remoteproc/Makefile
> >>  index 00f09e658cb3..6eb0137abbc7 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_RPROC)                        += ingenic_rproc.a
> >
> > This comes out as one tab to many on my side.
> >
> >>   obj-$(CONFIG_OMAP_REMOTEPROC)              += omap_remoteproc.o
> >>   obj-$(CONFIG_WKUP_M3_RPROC)                += wkup_m3_rproc.o
> >>   obj-$(CONFIG_DA8XX_REMOTEPROC)             += da8xx_remoteproc.o
> >>  diff --git a/drivers/remoteproc/ingenic_rproc.c
> >> b/drivers/remoteproc/ingenic_rproc.c
> >>  new file mode 100644
> >>  index 000000000000..5a7c7d8d9a90
> >>  --- /dev/null
> >>  +++ b/drivers/remoteproc/ingenic_rproc.c
> >>  @@ -0,0 +1,242 @@
> >>  +// SPDX-License-Identifier: GPL-2.0+
> >>  +/*
> >>  + * Ingenic JZ47xx remoteproc driver
> >>  + * Copyright 2019, Paul Cercueil <paul@crapouillou.net>
> >>  + */
> >>  +
> >>  +#include <linux/bitops.h>
> >>  +#include <linux/clk.h>
> >>  +#include <linux/err.h>
> >>  +#include <linux/interrupt.h>
> >>  +#include <linux/io.h>
> >>  +#include <linux/module.h>
> >>  +#include <linux/platform_device.h>
> >>  +#include <linux/remoteproc.h>
> >>  +
> >>  +#include "remoteproc_internal.h"
> >>  +
> >>  +#define REG_AUX_CTRL               0x0
> >>  +#define REG_AUX_MSG_ACK            0x10
> >>  +#define REG_AUX_MSG                0x14
> >>  +#define REG_CORE_MSG_ACK   0x18
> >>  +#define REG_CORE_MSG               0x1C
> >>  +
> >>  +#define AUX_CTRL_SLEEP             BIT(31)
> >>  +#define AUX_CTRL_MSG_IRQ_EN        BIT(3)
> >>  +#define AUX_CTRL_NMI_RESETS        BIT(2)
> >>  +#define AUX_CTRL_NMI               BIT(1)
> >>  +#define AUX_CTRL_SW_RESET  BIT(0)
> >>  +
> >>  +struct vpu_mem_map {
> >>  +   const char *name;
> >>  +   unsigned int da;
> >>  +};
> >>  +
> >>  +struct vpu_mem_info {
> >>  +   const struct vpu_mem_map *map;
> >>  +   unsigned long len;
> >>  +   void __iomem *base;
> >>  +};
> >>  +
> >>  +static const struct vpu_mem_map vpu_mem_map[] = {
> >>  +   { "tcsm0", 0x132b0000 },
> >>  +   { "tcsm1", 0xf4000000 },
> >>  +   { "sram",  0x132f0000 },
> >>  +};
> >>  +
> >>  +/* Device data */
> >>  +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;
> >>  +};
> >
> > Documentation of structure fields is always appreciated but it is up
> > to Bjorn to
> > decide on this one.
>
> It's a private structure within a driver, does it matter that much?
> Besides, it's pretty much self-explanatory.

It is a matter of time before people add complexity to the drivers and
more field in the structures.  If documentation is already present
then new fields get documented and things are easier to understand.

>
> >>  +
> >>  +static int ingenic_rproc_prepare(struct rproc *rproc)
> >>  +{
> >>  +   struct vpu *vpu = rproc->priv;
> >>  +   int ret;
> >>  +
> >>  +   /* The clocks must be enabled for the firmware to be loaded in
> >> TCSM */
> >>  +   ret = clk_bulk_prepare_enable(ARRAY_SIZE(vpu->clks), vpu->clks);
> >>  +   if (ret)
> >>  +           dev_err(vpu->dev, "Unable to start clocks: %d", ret);
> >>  +
> >>  +   return ret;
> >>  +}
> >>  +
> >>  +static void ingenic_rproc_unprepare(struct rproc *rproc)
> >>  +{
> >>  +   struct vpu *vpu = rproc->priv;
> >>  +
> >>  +   clk_bulk_disable_unprepare(ARRAY_SIZE(vpu->clks), vpu->clks);
> >>  +}
> >>  +
> >>  +static int ingenic_rproc_start(struct rproc *rproc)
> >>  +{
> >>  +   struct vpu *vpu = rproc->priv;
> >>  +   u32 ctrl;
> >>  +
> >>  +   enable_irq(vpu->irq);
> >>  +
> >>  +   /* Reset the AUX and enable message IRQ */
> >>  +   ctrl = AUX_CTRL_NMI_RESETS | AUX_CTRL_NMI | AUX_CTRL_MSG_IRQ_EN;
> >>  +   writel(ctrl, vpu->aux_base + REG_AUX_CTRL);
> >
> > Out of curiosity, there is no power domain associated with this
> > co-processor?  Clocks are all you need?
>
> That's correct.
>
> >>  +
> >>  +   return 0;
> >>  +}
> >>  +
> >>  +static int ingenic_rproc_stop(struct rproc *rproc)
> >>  +{
> >>  +   struct vpu *vpu = rproc->priv;
> >>  +
> >>  +   /* Keep AUX in reset mode */
> >>  +   writel(AUX_CTRL_SW_RESET, vpu->aux_base + REG_AUX_CTRL);
> >>  +
> >>  +   disable_irq_nosync(vpu->irq);
> >
> > Also out of curiosity, why the need for _nosync()?
>
> It probably doesn't need it. I'll remove it.
>
> >>  +
> >>  +   return 0;
> >>  +}
> >>  +
> >>  +static void ingenic_rproc_kick(struct rproc *rproc, int vqid)
> >>  +{
> >>  +   struct vpu *vpu = rproc->priv;
> >>  +
> >>  +   writel(vqid, vpu->aux_base + REG_CORE_MSG);
> >>  +}
> >>  +
> >>  +static void *ingenic_rproc_da_to_va(struct rproc *rproc, u64 da,
> >> int len)
> >>  +{
> >>  +   struct vpu *vpu = rproc->priv;
> >>  +   void __iomem *va = NULL;
> >>  +   unsigned int i;
> >>  +
> >>  +   if (len <= 0)
> >>  +           return NULL;
> >>  +
> >>  +   for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
> >>  +           const struct vpu_mem_info *info = &vpu->mem_info[i];
> >>  +           const struct vpu_mem_map *map = info->map;
> >>  +
> >>  +           if (da >= map->da && (da + len) < (map->da + info->len)) {
> >>  +                   va = info->base + (da - map->da);
> >>  +                   break;
> >>  +           }
> >>  +   }
> >>  +
> >>  +   return (__force void *)va;
> >>  +}
> >>  +
> >>  +static struct rproc_ops ingenic_rproc_ops = {
> >>  +   .prepare = ingenic_rproc_prepare,
> >>  +   .unprepare = ingenic_rproc_unprepare,
> >>  +   .start = ingenic_rproc_start,
> >>  +   .stop = ingenic_rproc_stop,
> >>  +   .kick = ingenic_rproc_kick,
> >>  +   .da_to_va = ingenic_rproc_da_to_va,
> >>  +};
> >>  +
> >>  +static irqreturn_t vpu_interrupt(int irq, void *data)
> >>  +{
> >>  +   struct rproc *rproc = data;
> >>  +   struct vpu *vpu = rproc->priv;
> >>  +   u32 vring;
> >>  +
> >>  +   vring = readl(vpu->aux_base + REG_AUX_MSG);
> >>  +
> >>  +   /* Ack the interrupt */
> >>  +   writel(0, vpu->aux_base + REG_AUX_MSG_ACK);
> >>  +
> >>  +   return rproc_vq_interrupt(rproc, vring);
> >>  +}
> >>  +
> >>  +static int ingenic_rproc_probe(struct platform_device *pdev)
> >>  +{
> >>  +   struct device *dev = &pdev->dev;
> >>  +   struct resource *mem;
> >>  +   struct rproc *rproc;
> >>  +   struct vpu *vpu;
> >>  +   unsigned int i;
> >>  +   int ret;
> >>  +
> >>  +   rproc = devm_rproc_alloc(dev, "ingenic-vpu",
> >>  +                            &ingenic_rproc_ops, NULL, sizeof(*vpu));
> >>  +   if (!rproc)
> >>  +           return -ENOMEM;
> >>  +
> >>  +   vpu = rproc->priv;
> >>  +   vpu->dev = &pdev->dev;
> >>  +
> >>  +   mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aux");
> >>  +   vpu->aux_base = devm_ioremap_resource(dev, mem);
> >>  +   if (IS_ERR(vpu->aux_base)) {
> >>  +           dev_err(dev, "Failed to ioremap");
> >>  +           return PTR_ERR(vpu->aux_base);
> >>  +   }
> >>  +
> >>  +   for (i = 0; i < ARRAY_SIZE(vpu_mem_map); i++) {
> >>  +           mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >>  +                                              vpu_mem_map[i].name);
> >>  +
> >>  +           vpu->mem_info[i].base = devm_ioremap_resource(dev, mem);
> >>  +           if (IS_ERR(vpu->mem_info[i].base)) {
> >>  +                   ret = PTR_ERR(vpu->mem_info[i].base);
> >>  +                   dev_err(dev, "Failed to ioremap");
> >>  +                   return ret;
> >>  +           }
> >>  +
> >>  +           vpu->mem_info[i].len = resource_size(mem);
> >>  +           vpu->mem_info[i].map = &vpu_mem_map[i];
> >>  +   }
> >>  +
> >>  +   vpu->clks[0].id = "vpu";
> >>  +   vpu->clks[1].id = "aux";
> >>  +
> >>  +   ret = devm_clk_bulk_get(dev, ARRAY_SIZE(vpu->clks), vpu->clks);
> >>  +   if (ret) {
> >>  +           dev_err(dev, "Failed to get clocks");
> >>  +           return ret;
> >>  +   }
> >>  +
> >>  +   vpu->irq = platform_get_irq(pdev, 0);
> >>  +   if (vpu->irq < 0) {
> >>  +           dev_err(dev, "Failed to get platform IRQ");
> >>  +           return vpu->irq;
> >>  +   }
> >>  +
> >>  +   ret = devm_request_irq(dev, vpu->irq, vpu_interrupt, 0, "VPU",
> >> rproc);
> >>  +   if (ret < 0) {
> >>  +           dev_err(dev, "Failed to request IRQ");
> >>  +           return ret;
> >>  +   }
> >>  +
> >>  +   disable_irq_nosync(vpu->irq);
> >>  +
> >>  +   ret = devm_rproc_add(dev, rproc);
> >>  +   if (ret) {
> >>  +           dev_err(dev, "Failed to register remote processor");
> >>  +           return ret;
> >>  +   }
> >>  +
> >>  +   return 0;
> >>  +}
> >>  +
> >>  +static const struct of_device_id ingenic_rproc_of_matches[] = {
> >>  +   { .compatible = "ingenic,jz4770-vpu-rproc", },
> >>  +   {}
> >>  +};
> >>  +MODULE_DEVICE_TABLE(of, ingenic_rproc_of_matches);
> >>  +
> >>  +static struct platform_driver ingenic_rproc_driver = {
> >>  +   .probe = ingenic_rproc_probe,
> >>  +   .driver = {
> >>  +           .name = "ingenic-vpu",
> >>  +           .of_match_table = of_match_ptr(ingenic_rproc_of_matches),
> >>  +   },
> >>  +};
> >>  +module_platform_driver(ingenic_rproc_driver);
> >>  +
> >>  +MODULE_LICENSE("GPL");
> >>  +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> >>  +MODULE_DESCRIPTION("Ingenic JZ47xx Remote Processor control
> >> driver");
> >
> > Nice, clean and simple driver.  With the above and after rolling in
> > Julia's
> > patch:
> >
> > Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>
> Thanks!
>
> Cheers,
> -Paul
>
>

Re: [PATCH v3 1/4] dt-bindings: Document JZ47xx VPU auxiliary processor

[Rob Herring]

On Sat, 16 Nov 2019 18:08:43 +0100, Paul Cercueil wrote:
> Inside the Video Processing Unit (VPU) of the recent JZ47xx SoCs from
> Ingenic is a second Xburst MIPS CPU very similar to the main core.
> This document describes the devicetree bindings for this auxiliary
> processor.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v2: Update TCSM0 address in example
>     v3: Change node name to 'video-decoder'
> 
>  .../bindings/remoteproc/ingenic,vpu.txt       | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/ingenic,vpu.txt
> 

Reviewed-by: Rob Herring <robh@kernel.org>