[PATCH v3 0/4] Add support of mt8183 APU

[PATCH v3 0/4] Add support of mt8183 APU

[Alexandre Bailon]

Some Mediatek's SoC have an Accelerated Processing Unit.
This adds support of the one available in the mt8183
(aswell some derivative SoC).

This v3 attempt to handle memory differently.
In the v2, the memory was reserved in dts and all dma allocation made by
remoteproc was using it. Since we may load many firmwares with different size,
this could lead to a memory waste or out of memory issues.
In addition, the APU itself expect the memory to mapped at a specific address.
The firmware, because of many hardware / software limitations use hardcoded
address.
To simplify memory management of userspace application, kernel and firmware,
and to rmeove a couple of hacks, we changed the way to manage the memory.
With the v3, only the virtio memory (that won't never change) is reserved
in the dts. The firmware declare resource similar to the CARVOUT_RSC 
that used by this driver to allocate and map the memory using the IOMMU.
I am not sure that the best solution but the simplest one I found so far.

Changes in v3:
- Remove IOMMU hack. Instead, manage the IOMMU directly from the driver
- Update the device tree bindings: only use reserved memory for virtio.
  All the other memory allocation will be done using DMA API.
  This sould simplify the memory management.
- Add more comments
Changes in v2:
- Drop the workarounds needed to load bad firmwares
- There are many name for the APU (most common one is VPU).
  Rename many functions and dts nodes to be more consistent.
- Use the bulk clock API, and enable / disable clock at a better place
- add few comments explaining how to start the APU
- update the way to use pinctl for JTAG
- fix some minors issues
- fix device tree bindings

Alexandre Bailon (4):
  dt bindings: remoteproc: Add bindings for MT8183 APU
  remoteproc: Add a remoteproc driver for the MT8183's APU
  remoteproc: mtk_vpu_rproc: Add support of JTAG
  ARM64: mt8183: Add support of APU to mt8183

 .../bindings/remoteproc/mtk,apu.yaml          | 118 ++++
 .../boot/dts/mediatek/mt8183-pumpkin.dts      |  48 ++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      |  40 ++
 drivers/remoteproc/Kconfig                    |  19 +
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/mtk_apu.c                  | 590 ++++++++++++++++++
 6 files changed, 816 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
 create mode 100644 drivers/remoteproc/mtk_apu.c

-- 
2.31.1

[PATCH v3 1/4] dt bindings: remoteproc: Add bindings for MT8183 APU

[Alexandre Bailon]

This adds dt bindings for the APU present in the MT8183.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 .../bindings/remoteproc/mtk,apu.yaml          | 118 ++++++++++++++++++
 1 file changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml b/Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
new file mode 100644
index 0000000000000..d5da6be66610c
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
@@ -0,0 +1,118 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+
+---
+$id: "http://devicetree.org/schemas/remoteproc/mtk,apu.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: MT8183 AI Processor Unit (APU) a.k.a. Vision Processor Unit (VPU)
+
+description:
+  This document defines the binding for the APU, a co-processor that could
+  offload the CPU for machine learning and neural network.
+
+maintainers:
+  - Alexandre Bailon <abailon@bayLibre.com>
+
+properties:
+  compatible:
+    const: mediatek,mt8183-apu
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    description:
+      Three clocks are expected for AXI, IPU and JTAG.
+      The JTAG clock seems to be required to run the DSP,
+      even when JTAG is not in use."
+    maxItems: 3
+
+  clock-names:
+    items:
+      - const: axi
+      - const: ipu
+      - const: jtag
+
+  iommus:
+    maxItems: 3
+
+  memory-region:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  pinctrl:
+    description: pinctrl handles, required to configure pins for JTAG.
+
+  pinctrl-names:
+    items:
+      - const: jtag
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - iommus
+  - memory-region
+  - power-domains
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/mt8183-clk.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/memory/mt8183-larb-port.h>
+    #include <dt-bindings/power/mt8183-power.h>
+
+    reserved-memory {
+      #address-cells = <1>;
+      #size-cells = <1>;
+      ranges;
+
+      vdev0vring0: vdev0vring0 {
+        compatible = "shared-dma-pool";
+        size = <0 0x00008000>;
+        no-map;
+      };
+
+      vdev0vring1: vdev0vring1 {
+        compatible = "shared-dma-pool";
+        size = <0 0x00008000>;
+        no-map;
+      };
+
+      vdev0buffer: vdev0buffer {
+        compatible = "shared-dma-pool";
+        size = <0 0x00100000>;
+        no-map;
+      };
+    };
+
+    apu0: apu@19100000 {
+      compatible = "mediatek,mt8183-apu";
+      reg = <0x19180000 0x14000>;
+      interrupts = <GIC_SPI 292 IRQ_TYPE_LEVEL_LOW>;
+
+      iommus = <&iommu M4U_PORT_IMG_IPUO>,
+         <&iommu M4U_PORT_IMG_IPU3O>,
+         <&iommu M4U_PORT_IMG_IPUI>;
+
+      clocks = <&ipu_core0 CLK_IPU_CORE0_AXI>,
+         <&ipu_core0 CLK_IPU_CORE0_IPU>,
+         <&ipu_core0 CLK_IPU_CORE0_JTAG>;
+
+      clock-names = "axi", "ipu", "jtag";
+
+      power-domains = <&scpsys MT8183_POWER_DOMAIN_VPU_CORE0>;
+      memory-region = <&vdev0buffer>, <&vdev0vring0>, <&vdev0vring1>;
+      memory-region-names = "vdev0buffer", "vdev0vring0", "vdev0vring1";
+    };
+...
-- 
2.31.1

[PATCH v3 2/4] remoteproc: Add a remoteproc driver for the MT8183's APU

[Alexandre Bailon]

This adds a driver to control the APU present in the MT8183.
This loads the firmware and start the DSP.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/remoteproc/Kconfig   |  10 +
 drivers/remoteproc/Makefile  |   1 +
 drivers/remoteproc/mtk_apu.c | 440 +++++++++++++++++++++++++++++++++++
 3 files changed, 451 insertions(+)
 create mode 100644 drivers/remoteproc/mtk_apu.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 9a6eedc3994a5..6c106a6c3ad5d 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -53,6 +53,16 @@ config MTK_SCP
 
 	  It's safe to say N here.
 
+config MTK_APU
+	tristate "Mediatek APU remoteproc support"
+	depends on ARCH_MEDIATEK
+	depends on MTK_IOMMU
+	help
+	  Say y to support the Mediatek's Accelerated Processing Unit (APU) via
+	  the remote processor framework.
+
+	  It's safe to say N here.
+
 config OMAP_REMOTEPROC
 	tristate "OMAP remoteproc support"
 	depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index bb26c9e4ef9cf..e395af86c17b0 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.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_MTK_APU)			+= mtk_apu.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/mtk_apu.c b/drivers/remoteproc/mtk_apu.c
new file mode 100644
index 0000000000000..0e3f63987bd85
--- /dev/null
+++ b/drivers/remoteproc/mtk_apu.c
@@ -0,0 +1,440 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 BayLibre SAS
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/iommu.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+#define SW_RST					(0x0000000C)
+#define SW_RST_OCD_HALT_ON_RST			BIT(12)
+#define SW_RST_IPU_D_RST			BIT(8)
+#define SW_RST_IPU_B_RST			BIT(4)
+#define CORE_CTRL				(0x00000110)
+#define CORE_CTRL_PDEBUG_ENABLE			BIT(31)
+#define CORE_CTRL_SRAM_64K_iMEM			(0x00 << 27)
+#define CORE_CTRL_SRAM_96K_iMEM			(0x01 << 27)
+#define CORE_CTRL_SRAM_128K_iMEM		(0x02 << 27)
+#define CORE_CTRL_SRAM_192K_iMEM		(0x03 << 27)
+#define CORE_CTRL_SRAM_256K_iMEM		(0x04 << 27)
+#define CORE_CTRL_PBCLK_ENABLE			BIT(26)
+#define CORE_CTRL_RUN_STALL			BIT(23)
+#define CORE_CTRL_STATE_VECTOR_SELECT		BIT(19)
+#define CORE_CTRL_PIF_GATED			BIT(17)
+#define CORE_CTRL_NMI				BIT(0)
+#define CORE_XTENSA_INT				(0x00000114)
+#define CORE_CTL_XTENSA_INT			(0x00000118)
+#define CORE_DEFAULT0				(0x0000013C)
+#define CORE_DEFAULT0_QOS_SWAP_0		(0x00 << 28)
+#define CORE_DEFAULT0_QOS_SWAP_1		(0x01 << 28)
+#define CORE_DEFAULT0_QOS_SWAP_2		(0x02 << 28)
+#define CORE_DEFAULT0_QOS_SWAP_3		(0x03 << 28)
+#define CORE_DEFAULT0_ARUSER_USE_IOMMU		(0x10 << 23)
+#define CORE_DEFAULT0_AWUSER_USE_IOMMU		(0x10 << 18)
+#define CORE_DEFAULT1				(0x00000140)
+#define CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU	(0x10 << 0)
+#define CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU	(0x10 << 5)
+#define CORE_XTENSA_ALTRESETVEC			(0x000001F8)
+
+#define RSC_VENDOR_CARVEOUT			(RSC_VENDOR_START + 1)
+
+#define APU_RESET_DELAY				(27)
+
+struct mtk_apu_rproc {
+	struct device *dev;
+	void __iomem *base;
+	int irq;
+	struct clk_bulk_data clks[3];
+	struct list_head mappings;
+};
+
+static int mtk_apu_iommu_map(struct rproc *rproc, struct rproc_mem_entry *entry)
+{
+	struct mtk_apu_rproc *apu_rproc = rproc->priv;
+	struct device *dev = rproc->dev.parent;
+	struct rproc_mem_entry *mapping;
+	struct iommu_domain *domain;
+	int ret;
+	u64 pa;
+
+	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
+	if (!mapping)
+		return -ENOMEM;
+
+	if (!entry->va)
+		pa = entry->dma;
+	else
+		pa = rproc_va_to_pa(entry->va);
+
+	if ((strcmp(entry->name, "vdev0vring0") == 0 ||
+		strcmp(entry->name, "vdev0vring1") == 0)) {
+		entry->va = memremap(entry->dma, entry->len, MEMREMAP_WC);
+		if (IS_ERR_OR_NULL(entry->va)) {
+			dev_err(dev, "Unable to map memory region: %pa+%lx\n",
+				&entry->dma, entry->len);
+			ret = PTR_ERR(mapping->va);
+			goto free_mapping;
+		}
+		mapping->va = entry->va;
+	}
+
+	domain = iommu_get_domain_for_dev(dev);
+	ret = iommu_map(domain, entry->da, pa, entry->len, entry->flags);
+	if (ret) {
+		dev_err(dev, "iommu_map failed: %d\n", ret);
+		goto err_memunmap;
+	}
+
+	mapping->da = entry->da;
+	mapping->len = entry->len;
+	list_add_tail(&mapping->node, &apu_rproc->mappings);
+
+	return 0;
+
+err_memunmap:
+	memunmap(mapping->va);
+free_mapping:
+	kfree(mapping);
+
+	return ret;
+}
+
+static void mtk_apu_iommu_unmap_all(struct rproc *rproc)
+{
+	struct mtk_apu_rproc *apu_rproc = rproc->priv;
+	struct device *dev = rproc->dev.parent;
+	struct rproc_mem_entry *entry, *tmp;
+	struct iommu_domain *domain;
+
+	/* clean up iommu mapping entries */
+	list_for_each_entry_safe(entry, tmp, &apu_rproc->mappings, node) {
+		size_t unmapped;
+
+		domain = iommu_get_domain_for_dev(dev);
+		unmapped = iommu_unmap(domain, entry->da, entry->len);
+		if (unmapped != entry->len) {
+			/* nothing much to do besides complaining */
+			dev_err(dev, "failed to unmap %zx/%zu\n", entry->len,
+				unmapped);
+		}
+		memunmap(entry->va);
+
+		list_del(&entry->node);
+		kfree(entry);
+	}
+}
+
+static int mtk_apu_rproc_prepare(struct rproc *rproc)
+{
+	struct mtk_apu_rproc *apu_rproc = rproc->priv;
+	int ret;
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(apu_rproc->clks),
+				      apu_rproc->clks);
+	if (ret)
+		dev_err(apu_rproc->dev, "Failed to enable clocks\n");
+
+	return ret;
+}
+
+static int mtk_apu_rproc_unprepare(struct rproc *rproc)
+{
+	struct mtk_apu_rproc *apu_rproc = rproc->priv;
+
+	clk_bulk_disable_unprepare(ARRAY_SIZE(apu_rproc->clks),
+				   apu_rproc->clks);
+
+	return 0;
+}
+
+static int mtk_apu_rproc_start(struct rproc *rproc)
+{
+	struct mtk_apu_rproc *apu_rproc = rproc->priv;
+	u32 core_ctrl;
+
+	/* Set reset vector of APU firmware boot address */
+	writel(rproc->bootaddr, apu_rproc->base + CORE_XTENSA_ALTRESETVEC);
+
+	/* Turn on the clocks and stall the APU */
+	core_ctrl = readl(apu_rproc->base + CORE_CTRL);
+	core_ctrl |= CORE_CTRL_PDEBUG_ENABLE | CORE_CTRL_PBCLK_ENABLE |
+		     CORE_CTRL_STATE_VECTOR_SELECT | CORE_CTRL_RUN_STALL |
+		     CORE_CTRL_PIF_GATED;
+	writel(core_ctrl, apu_rproc->base + CORE_CTRL);
+
+	/* Reset the APU: this requires 27 ns to be effective on any platform */
+	writel(SW_RST_OCD_HALT_ON_RST | SW_RST_IPU_B_RST | SW_RST_IPU_D_RST,
+		apu_rproc->base + SW_RST);
+	ndelay(APU_RESET_DELAY);
+	writel(0, apu_rproc->base + SW_RST);
+
+	core_ctrl &= ~CORE_CTRL_PIF_GATED;
+	writel(core_ctrl, apu_rproc->base + CORE_CTRL);
+
+	/* Configure memory accesses to go through the IOMMU */
+	writel(CORE_DEFAULT0_AWUSER_USE_IOMMU | CORE_DEFAULT0_ARUSER_USE_IOMMU |
+	      CORE_DEFAULT0_QOS_SWAP_1, apu_rproc->base + CORE_DEFAULT0);
+	writel(CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU |
+		CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU,
+		apu_rproc->base + CORE_DEFAULT1);
+
+	/* Release the APU */
+	core_ctrl &= ~CORE_CTRL_RUN_STALL;
+	writel(core_ctrl, apu_rproc->base + CORE_CTRL);
+
+	return 0;
+}
+
+static int mtk_apu_rproc_stop(struct rproc *rproc)
+{
+	struct mtk_apu_rproc *apu_rproc = rproc->priv;
+	u32 core_ctrl;
+
+	core_ctrl = readl(apu_rproc->base + CORE_CTRL);
+	writel(core_ctrl | CORE_CTRL_RUN_STALL, apu_rproc->base + CORE_CTRL);
+
+	mtk_apu_iommu_unmap_all(rproc);
+
+	return 0;
+}
+
+static void mtk_apu_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct mtk_apu_rproc *apu_rproc = rproc->priv;
+
+	writel(1 << vqid, apu_rproc->base + CORE_CTL_XTENSA_INT);
+}
+
+static int mtk_apu_load(struct rproc *rproc, const struct firmware *fw)
+
+{
+	struct rproc_mem_entry *entry, *tmp;
+	int ret;
+
+	ret = rproc_elf_load_segments(rproc, fw);
+	if (ret)
+		return ret;
+
+	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
+		ret = mtk_apu_iommu_map(rproc, entry);
+		if (ret)
+			goto err_unmap_all;
+	}
+
+	return 0;
+
+err_unmap_all:
+	mtk_apu_iommu_unmap_all(rproc);
+
+	return ret;
+}
+
+static struct reserved_mem *of_reserved_mem_by_name(struct rproc *rproc,
+						    const char *name)
+{
+	struct device *dev = rproc->dev.parent;
+	struct device_node *np = dev->of_node;
+	struct device_node *target;
+	struct reserved_mem *rmem;
+	int idx;
+
+	idx = of_property_match_string(np, "memory-region-names", name);
+	if (idx < 0) {
+		dev_err(dev, "failed to find %s memory\n", name);
+		return NULL;
+	}
+
+	target = of_parse_phandle(np, "memory-region", idx);
+	if (!target)
+		return NULL;
+
+	rmem = of_reserved_mem_lookup(target);
+	if (!rmem)
+		dev_err(dev, "unable to acquire memory-region\n");
+	of_node_put(target);
+
+	return rmem;
+}
+
+static int mtk_apu_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
+			      int offset, int avail)
+{
+	struct device *dev = rproc->dev.parent;
+
+	if (rsc_type == RSC_VENDOR_CARVEOUT) {
+		struct fw_rsc_carveout *rsc_carveout = rsc;
+		struct rproc_mem_entry *mem;
+		struct reserved_mem *rmem;
+
+		rmem = of_reserved_mem_by_name(rproc, rsc_carveout->name);
+		if (!rmem)
+			return -ENOMEM;
+
+		if (rmem->size < rsc_carveout->len) {
+			dev_err(dev, "The reserved memory is too small\n");
+			return -ENOMEM;
+		}
+
+		mem = rproc_mem_entry_init(dev, NULL, (dma_addr_t)rmem->base,
+					   rsc_carveout->len, rsc_carveout->da,
+					   NULL, NULL, rsc_carveout->name);
+		if (!mem)
+			return -ENOMEM;
+
+		mem->flags = rsc_carveout->flags;
+		rsc_carveout->pa = rmem->base;
+		rproc_add_carveout(rproc, mem);
+	}
+
+	return RSC_HANDLED;
+}
+
+static const struct rproc_ops mtk_apu_rproc_ops = {
+	.prepare		= mtk_apu_rproc_prepare,
+	.unprepare		= mtk_apu_rproc_unprepare,
+	.start			= mtk_apu_rproc_start,
+	.stop			= mtk_apu_rproc_stop,
+	.kick			= mtk_apu_rproc_kick,
+	.load			= mtk_apu_load,
+	.parse_fw		= rproc_elf_load_rsc_table,
+	.find_loaded_rsc_table	= rproc_elf_find_loaded_rsc_table,
+	.sanity_check		= rproc_elf_sanity_check,
+	.get_boot_addr		= rproc_elf_get_boot_addr,
+	.handle_rsc		= mtk_apu_handle_rsc,
+};
+
+static irqreturn_t mtk_apu_rproc_callback(int irq, void *data)
+{
+	struct rproc *rproc = data;
+	struct mtk_apu_rproc *apu_rproc = (struct mtk_apu_rproc *)rproc->priv;
+
+	writel(1, apu_rproc->base + CORE_XTENSA_INT);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t handle_event(int irq, void *data)
+{
+	struct rproc *rproc = data;
+
+	rproc_vq_interrupt(rproc, 0);
+	rproc_vq_interrupt(rproc, 1);
+
+	return IRQ_HANDLED;
+}
+
+static int mtk_apu_rproc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_apu_rproc *apu_rproc;
+	struct rproc *rproc;
+	struct resource *res;
+	int ret;
+
+	rproc = rproc_alloc(dev, dev_name(dev), &mtk_apu_rproc_ops, NULL,
+			    sizeof(*apu_rproc));
+	if (!rproc)
+		return -ENOMEM;
+
+	rproc->recovery_disabled = true;
+	rproc->has_iommu = false;
+	rproc->auto_boot = false;
+
+	apu_rproc = rproc->priv;
+	apu_rproc->dev = dev;
+	INIT_LIST_HEAD(&apu_rproc->mappings);
+
+	platform_set_drvdata(pdev, rproc);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	apu_rproc->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(apu_rproc->base)) {
+		dev_err(dev, "Failed to map mmio\n");
+		ret = PTR_ERR(apu_rproc->base);
+		goto free_rproc;
+	}
+
+	apu_rproc->irq = platform_get_irq(pdev, 0);
+	if (apu_rproc->irq < 0) {
+		ret = apu_rproc->irq;
+		goto free_rproc;
+	}
+
+	ret = devm_request_threaded_irq(dev, apu_rproc->irq,
+					mtk_apu_rproc_callback, handle_event,
+					IRQF_SHARED | IRQF_ONESHOT,
+					NULL, rproc);
+	if (ret) {
+		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
+		goto free_rproc;
+	}
+
+	apu_rproc->clks[0].id = "ipu";
+	apu_rproc->clks[1].id = "axi";
+	apu_rproc->clks[2].id = "jtag";
+
+	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(apu_rproc->clks),
+				apu_rproc->clks);
+	if (ret) {
+		dev_err(dev, "Failed to get clocks\n");
+		goto free_rproc;
+	}
+
+	ret = rproc_add(rproc);
+	if (ret) {
+		dev_err(dev, "rproc_add failed: %d\n", ret);
+		goto free_rproc;
+	}
+
+	return 0;
+
+free_rproc:
+	rproc_free(rproc);
+
+	return ret;
+}
+
+static int mtk_apu_rproc_remove(struct platform_device *pdev)
+{
+	struct rproc *rproc = platform_get_drvdata(pdev);
+	struct mtk_apu_rproc *apu_rproc = (struct mtk_apu_rproc *)rproc->priv;
+	struct device *dev = &pdev->dev;
+
+	disable_irq(apu_rproc->irq);
+
+	rproc_del(rproc);
+	of_reserved_mem_device_release(dev);
+	rproc_free(rproc);
+
+	return 0;
+}
+
+static const struct of_device_id mtk_apu_rproc_of_match[] = {
+	{ .compatible = "mediatek,mt8183-apu", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mtk_apu_rproc_of_match);
+
+static struct platform_driver mtk_apu_rproc_driver = {
+	.probe = mtk_apu_rproc_probe,
+	.remove = mtk_apu_rproc_remove,
+	.driver = {
+		.name = "mtk_apu-rproc",
+		.of_match_table = of_match_ptr(mtk_apu_rproc_of_match),
+	},
+};
+module_platform_driver(mtk_apu_rproc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Alexandre Bailon");
+MODULE_DESCRIPTION("MTK APU Remote Processor control driver");
-- 
2.31.1

[PATCH v3 3/4] remoteproc: mtk_vpu_rproc: Add support of JTAG

[Alexandre Bailon]

The DSP could be debugged using JTAG.
The support of JTAG could enabled at build time and it could be enabled
using debugfs.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/remoteproc/Kconfig   |   9 +++
 drivers/remoteproc/mtk_apu.c | 147 ++++++++++++++++++++++++++++++++++-
 2 files changed, 155 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 6c106a6c3ad5d..cb1a2d9f4a9f6 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -63,6 +63,15 @@ config MTK_APU
 
 	  It's safe to say N here.
 
+config MTK_APU_JTAG
+	bool "Enable support of JTAG"
+	depends on MTK_APU
+	help
+	  Say y to enable support of JTAG.
+	  By default, JTAG will remain disabled until it is enabled using
+	  debugfs: remoteproc/remoteproc0/jtag. Write 1 to enable it and
+	  0 to disable it.
+
 config OMAP_REMOTEPROC
 	tristate "OMAP remoteproc support"
 	depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX
diff --git a/drivers/remoteproc/mtk_apu.c b/drivers/remoteproc/mtk_apu.c
index 0e3f63987bd85..3daed38bb5188 100644
--- a/drivers/remoteproc/mtk_apu.c
+++ b/drivers/remoteproc/mtk_apu.c
@@ -5,12 +5,14 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/iommu.h>
 #include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/of_reserved_mem.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/remoteproc.h>
 
@@ -44,6 +46,11 @@
 #define CORE_DEFAULT1				(0x00000140)
 #define CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU	(0x10 << 0)
 #define CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU	(0x10 << 5)
+#define CORE_DEFAULT2				(0x00000144)
+#define CORE_DEFAULT2_DBG_EN			BIT(3)
+#define CORE_DEFAULT2_NIDEN			BIT(2)
+#define CORE_DEFAULT2_SPNIDEN			BIT(1)
+#define CORE_DEFAULT2_SPIDEN			BIT(0)
 #define CORE_XTENSA_ALTRESETVEC			(0x000001F8)
 
 #define RSC_VENDOR_CARVEOUT			(RSC_VENDOR_START + 1)
@@ -56,6 +63,13 @@ struct mtk_apu_rproc {
 	int irq;
 	struct clk_bulk_data clks[3];
 	struct list_head mappings;
+
+#ifdef CONFIG_MTK_APU_JTAG
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pinctrl_jtag;
+	bool jtag_enabled;
+	struct mutex jtag_mutex;
+#endif
 };
 
 static int mtk_apu_iommu_map(struct rproc *rproc, struct rproc_mem_entry *entry)
@@ -333,6 +347,133 @@ static irqreturn_t handle_event(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_MTK_APU_JTAG
+
+static int apu_enable_jtag(struct mtk_apu_rproc *apu_rproc)
+{
+	int ret = 0;
+
+	mutex_lock(&apu_rproc->jtag_mutex);
+	if (apu_rproc->jtag_enabled)
+		goto err_mutex_unlock;
+
+	writel(CORE_DEFAULT2_SPNIDEN | CORE_DEFAULT2_SPIDEN |
+		CORE_DEFAULT2_NIDEN | CORE_DEFAULT2_DBG_EN,
+		apu_rproc->base + CORE_DEFAULT2);
+
+	apu_rproc->jtag_enabled = 1;
+
+err_mutex_unlock:
+	mutex_unlock(&apu_rproc->jtag_mutex);
+
+	return ret;
+}
+
+static int apu_disable_jtag(struct mtk_apu_rproc *apu_rproc)
+{
+	int ret = 0;
+
+	mutex_lock(&apu_rproc->jtag_mutex);
+	if (!apu_rproc->jtag_enabled)
+		goto err_mutex_unlock;
+
+	writel(0, apu_rproc->base + CORE_DEFAULT2);
+
+	apu_rproc->jtag_enabled = 0;
+
+err_mutex_unlock:
+	mutex_unlock(&apu_rproc->jtag_mutex);
+
+	return ret;
+}
+
+static ssize_t rproc_jtag_read(struct file *filp, char __user *userbuf,
+			       size_t count, loff_t *ppos)
+{
+	struct rproc *rproc = filp->private_data;
+	struct mtk_apu_rproc *apu_rproc = (struct mtk_apu_rproc *)rproc->priv;
+	char *buf = apu_rproc->jtag_enabled ? "enabled\n" : "disabled\n";
+
+	return simple_read_from_buffer(userbuf, count, ppos, buf, strlen(buf));
+}
+
+static ssize_t rproc_jtag_write(struct file *filp, const char __user *user_buf,
+				size_t count, loff_t *ppos)
+{
+	struct rproc *rproc = filp->private_data;
+	struct mtk_apu_rproc *apu_rproc = (struct mtk_apu_rproc *)rproc->priv;
+	char buf[10];
+	int ret;
+
+	if (count < 1 || count > sizeof(buf))
+		return -EINVAL;
+
+	ret = copy_from_user(buf, user_buf, count);
+	if (ret)
+		return -EFAULT;
+
+	/* remove end of line */
+	if (buf[count - 1] == '\n')
+		buf[count - 1] = '\0';
+
+	if (!strncmp(buf, "enabled", count))
+		ret = apu_enable_jtag(apu_rproc);
+	else if (!strncmp(buf, "disabled", count))
+		ret = apu_disable_jtag(apu_rproc);
+	else
+		return -EINVAL;
+
+	return ret ? ret : count;
+}
+
+static const struct file_operations rproc_jtag_ops = {
+	.read = rproc_jtag_read,
+	.write = rproc_jtag_write,
+	.open = simple_open,
+};
+
+static int apu_jtag_probe(struct mtk_apu_rproc *apu_rproc)
+{
+	int ret;
+
+	if (!apu_rproc->rproc->dbg_dir)
+		return -ENODEV;
+
+	apu_rproc->pinctrl = devm_pinctrl_get(apu_rproc->dev);
+	if (IS_ERR(apu_rproc->pinctrl)) {
+		dev_warn(apu_rproc->dev, "Failed to find JTAG pinctrl\n");
+		return PTR_ERR(apu_rproc->pinctrl);
+	}
+
+	apu_rproc->pinctrl_jtag = pinctrl_lookup_state(apu_rproc->pinctrl,
+						       "jtag");
+	if (IS_ERR(apu_rproc->pinctrl_jtag))
+		return PTR_ERR(apu_rproc->pinctrl_jtag);
+
+	ret = pinctrl_select_state(apu_rproc->pinctrl,
+				   apu_rproc->pinctrl_jtag);
+	if (ret < 0)
+		return ret;
+
+	mutex_init(&apu_rproc->jtag_mutex);
+
+	debugfs_create_file("jtag", 0600, apu_rproc->rproc->dbg_dir,
+			    apu_rproc->rproc, &rproc_jtag_ops);
+
+	return 0;
+}
+#else
+static int apu_jtag_probe(struct mtk_apu_rproc *apu_rproc)
+{
+	return 0;
+}
+
+static int apu_disable_jtag(struct mtk_apu_rproc *apu_rproc)
+{
+	return 0;
+}
+#endif /* CONFIG_MTK_APU_JTAG */
+
 static int mtk_apu_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -396,6 +537,10 @@ static int mtk_apu_rproc_probe(struct platform_device *pdev)
 		goto free_rproc;
 	}
 
+	ret = apu_jtag_probe(apu_rproc);
+	if (ret)
+		dev_warn(dev, "Failed to configure jtag\n");
+
 	return 0;
 
 free_rproc:
@@ -411,7 +556,7 @@ static int mtk_apu_rproc_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 
 	disable_irq(apu_rproc->irq);
-
+	apu_disable_jtag(apu_rproc);
 	rproc_del(rproc);
 	of_reserved_mem_device_release(dev);
 	rproc_free(rproc);
-- 
2.31.1

[PATCH v3 4/4] ARM64: mt8183: Add support of APU to mt8183

[Alexandre Bailon]

This adds the support of APU to mt8183.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 .../boot/dts/mediatek/mt8183-pumpkin.dts      | 48 +++++++++++++++++++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 40 ++++++++++++++++
 2 files changed, 88 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts b/arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts
index ee912825cfc60..7fbed2b7bc6f8 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts
@@ -37,6 +37,42 @@ scp_mem_reserved: scp_mem_region@50000000 {
 			reg = <0 0x50000000 0 0x2900000>;
 			no-map;
 		};
+
+		vdev0vring0: vdev0vring0 {
+			compatible = "shared-dma-pool";
+			size = <0 0x00008000>;
+			no-map;
+		};
+
+		vdev0vring1: vdev0vring1 {
+			compatible = "shared-dma-pool";
+			size = <0 0x00008000>;
+			no-map;
+		};
+
+		vdev0buffer: vdev0buffer {
+			compatible = "shared-dma-pool";
+			size = <0 0x00100000>;
+			no-map;
+		};
+
+		vdev1vring0: vdev1vring0 {
+			compatible = "shared-dma-pool";
+			size = <0 0x00008000>;
+			no-map;
+		};
+
+		vdev1vring1: vdev1vring1 {
+			compatible = "shared-dma-pool";
+			size = <0 0x00008000>;
+			no-map;
+		};
+
+		vdev1buffer: vdev1buffer {
+			compatible = "shared-dma-pool";
+			size = <0 0x00100000>;
+			no-map;
+		};
 	};
 
 	leds {
@@ -381,3 +417,15 @@ &scp {
 &dsi0 {
 	status = "disabled";
 };
+
+&apu0 {
+	memory-region = <&vdev0buffer>, <&vdev0vring0>, <&vdev0vring1>;
+	memory-region-names = "vdev0buffer", "vdev0vring0", "vdev0vring1";
+	status = "okay";
+};
+
+&apu1 {
+	memory-region = <&vdev1buffer>, <&vdev1vring0>, <&vdev1vring1>;
+	memory-region-names = "vdev0buffer", "vdev0vring0", "vdev0vring1";
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index f90df6439c088..bf3f315ad3b2f 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -1447,12 +1447,52 @@ ipu_adl: syscon@19010000 {
 			#clock-cells = <1>;
 		};
 
+		apu0: apu@0x19100000 {
+			compatible = "mediatek,mt8183-apu";
+			reg = <0 0x19180000 0 0x14000>;
+			interrupts = <GIC_SPI 292 IRQ_TYPE_LEVEL_LOW>;
+
+			iommus = <&iommu M4U_PORT_IMG_IPUO>,
+				 <&iommu M4U_PORT_IMG_IPU3O>,
+				 <&iommu M4U_PORT_IMG_IPUI>;
+
+			clocks = <&ipu_core0 CLK_IPU_CORE0_AXI>,
+				 <&ipu_core0 CLK_IPU_CORE0_IPU>,
+				 <&ipu_core0 CLK_IPU_CORE0_JTAG>;
+
+			clock-names = "axi", "ipu", "jtag";
+
+			power-domains = <&spm MT8183_POWER_DOMAIN_VPU_CORE0>;
+
+			status = "disabled";
+		};
+
 		ipu_core0: syscon@19180000 {
 			compatible = "mediatek,mt8183-ipu_core0", "syscon";
 			reg = <0 0x19180000 0 0x1000>;
 			#clock-cells = <1>;
 		};
 
+		apu1: apu@19200000 {
+			compatible = "mediatek,mt8183-apu";
+			reg = <0 0x19280000 0 0x14000>;
+			interrupts = <GIC_SPI 293 IRQ_TYPE_LEVEL_LOW>;
+
+			iommus = <&iommu M4U_PORT_CAM_IPUO>,
+				 <&iommu M4U_PORT_CAM_IPU2O>,
+				 <&iommu M4U_PORT_CAM_IPU3O>;
+
+			clocks = <&ipu_core0 CLK_IPU_CORE1_AXI>,
+				 <&ipu_core0 CLK_IPU_CORE1_IPU>,
+				 <&ipu_core0 CLK_IPU_CORE1_JTAG>;
+
+			clock-names = "axi", "ipu", "jtag";
+
+			power-domains = <&spm MT8183_POWER_DOMAIN_VPU_CORE1>;
+
+			status = "disabled";
+		};
+
 		ipu_core1: syscon@19280000 {
 			compatible = "mediatek,mt8183-ipu_core1", "syscon";
 			reg = <0 0x19280000 0 0x1000>;
-- 
2.31.1

Re: [PATCH v3 1/4] dt bindings: remoteproc: Add bindings for MT8183 APU

[Rob Herring]

On Thu, 19 Aug 2021 17:13:37 +0200, Alexandre Bailon wrote:
> This adds dt bindings for the APU present in the MT8183.
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  .../bindings/remoteproc/mtk,apu.yaml          | 118 ++++++++++++++++++
>  1 file changed, 118 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/remoteproc/mtk,apu.example.dt.yaml:0:0: /example-0/reserved-memory/vdev0vring0: failed to match any schema with compatible: ['shared-dma-pool']
Documentation/devicetree/bindings/remoteproc/mtk,apu.example.dt.yaml:0:0: /example-0/reserved-memory/vdev0vring1: failed to match any schema with compatible: ['shared-dma-pool']
Documentation/devicetree/bindings/remoteproc/mtk,apu.example.dt.yaml:0:0: /example-0/reserved-memory/vdev0buffer: failed to match any schema with compatible: ['shared-dma-pool']
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/mtk,apu.example.dt.yaml: apu@19100000: memory-region: [[1], [2], [3]] is too long
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/mtk,apu.example.dt.yaml: apu@19100000: Additional properties are not allowed ('memory-region-names' was unexpected)
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1518714

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 2/4] remoteproc: Add a remoteproc driver for the MT8183's APU

[kernel test robot]

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

Hi Alexandre,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v5.14-rc6 next-20210819]
[cannot apply to remoteproc/for-next rpmsg/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Alexandre-Bailon/Add-support-of-mt8183-APU/20210819-231419
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/c161c7d11a0fdf701085657c15f949f397ade5be
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Alexandre-Bailon/Add-support-of-mt8183-APU/20210819-231419
        git checkout c161c7d11a0fdf701085657c15f949f397ade5be
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/iommu.h:11,
                    from drivers/remoteproc/mtk_apu.c:10:
   drivers/remoteproc/mtk_apu.c: In function 'mtk_apu_iommu_map':
>> drivers/remoteproc/mtk_apu.c:83:38: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
      83 |                         dev_err(dev, "Unable to map memory region: %pa+%lx\n",
         |                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
      19 | #define dev_fmt(fmt) fmt
         |                      ^~~
   drivers/remoteproc/mtk_apu.c:83:25: note: in expansion of macro 'dev_err'
      83 |                         dev_err(dev, "Unable to map memory region: %pa+%lx\n",
         |                         ^~~~~~~
   drivers/remoteproc/mtk_apu.c:83:74: note: format string is defined here
      83 |                         dev_err(dev, "Unable to map memory region: %pa+%lx\n",
         |                                                                        ~~^
         |                                                                          |
         |                                                                          long unsigned int
         |                                                                        %x


vim +83 drivers/remoteproc/mtk_apu.c

    60	
    61	static int mtk_apu_iommu_map(struct rproc *rproc, struct rproc_mem_entry *entry)
    62	{
    63		struct mtk_apu_rproc *apu_rproc = rproc->priv;
    64		struct device *dev = rproc->dev.parent;
    65		struct rproc_mem_entry *mapping;
    66		struct iommu_domain *domain;
    67		int ret;
    68		u64 pa;
    69	
    70		mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
    71		if (!mapping)
    72			return -ENOMEM;
    73	
    74		if (!entry->va)
    75			pa = entry->dma;
    76		else
    77			pa = rproc_va_to_pa(entry->va);
    78	
    79		if ((strcmp(entry->name, "vdev0vring0") == 0 ||
    80			strcmp(entry->name, "vdev0vring1") == 0)) {
    81			entry->va = memremap(entry->dma, entry->len, MEMREMAP_WC);
    82			if (IS_ERR_OR_NULL(entry->va)) {
  > 83				dev_err(dev, "Unable to map memory region: %pa+%lx\n",
    84					&entry->dma, entry->len);
    85				ret = PTR_ERR(mapping->va);
    86				goto free_mapping;
    87			}
    88			mapping->va = entry->va;
    89		}
    90	
    91		domain = iommu_get_domain_for_dev(dev);
    92		ret = iommu_map(domain, entry->da, pa, entry->len, entry->flags);
    93		if (ret) {
    94			dev_err(dev, "iommu_map failed: %d\n", ret);
    95			goto err_memunmap;
    96		}
    97	
    98		mapping->da = entry->da;
    99		mapping->len = entry->len;
   100		list_add_tail(&mapping->node, &apu_rproc->mappings);
   101	
   102		return 0;
   103	
   104	err_memunmap:
   105		memunmap(mapping->va);
   106	free_mapping:
   107		kfree(mapping);
   108	
   109		return ret;
   110	}
   111	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 78470 bytes --]

Re: [PATCH v3 2/4] remoteproc: Add a remoteproc driver for the MT8183's APU

[Mathieu Poirier]

Hi Alex,

On Thu, Aug 19, 2021 at 05:13:38PM +0200, Alexandre Bailon wrote:
> This adds a driver to control the APU present in the MT8183.
> This loads the firmware and start the DSP.
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/remoteproc/Kconfig   |  10 +
>  drivers/remoteproc/Makefile  |   1 +
>  drivers/remoteproc/mtk_apu.c | 440 +++++++++++++++++++++++++++++++++++
>  3 files changed, 451 insertions(+)
>  create mode 100644 drivers/remoteproc/mtk_apu.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index 9a6eedc3994a5..6c106a6c3ad5d 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -53,6 +53,16 @@ config MTK_SCP
>  
>  	  It's safe to say N here.
>  
> +config MTK_APU
> +	tristate "Mediatek APU remoteproc support"
> +	depends on ARCH_MEDIATEK
> +	depends on MTK_IOMMU
> +	help
> +	  Say y to support the Mediatek's Accelerated Processing Unit (APU) via

In the bindings A in APU is for AI or Vision whereas above it is for
accelerated.  Please pick one and stick with it.

> +	  the remote processor framework.
> +
> +	  It's safe to say N here.
> +
>  config OMAP_REMOTEPROC
>  	tristate "OMAP remoteproc support"
>  	depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index bb26c9e4ef9cf..e395af86c17b0 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.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_MTK_APU)			+= mtk_apu.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/mtk_apu.c b/drivers/remoteproc/mtk_apu.c
> new file mode 100644
> index 0000000000000..0e3f63987bd85
> --- /dev/null
> +++ b/drivers/remoteproc/mtk_apu.c
> @@ -0,0 +1,440 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 BayLibre SAS
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/iommu.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +
> +#include "remoteproc_internal.h"
> +
> +#define SW_RST					(0x0000000C)
> +#define SW_RST_OCD_HALT_ON_RST			BIT(12)
> +#define SW_RST_IPU_D_RST			BIT(8)
> +#define SW_RST_IPU_B_RST			BIT(4)
> +#define CORE_CTRL				(0x00000110)
> +#define CORE_CTRL_PDEBUG_ENABLE			BIT(31)
> +#define CORE_CTRL_SRAM_64K_iMEM			(0x00 << 27)
> +#define CORE_CTRL_SRAM_96K_iMEM			(0x01 << 27)
> +#define CORE_CTRL_SRAM_128K_iMEM		(0x02 << 27)
> +#define CORE_CTRL_SRAM_192K_iMEM		(0x03 << 27)
> +#define CORE_CTRL_SRAM_256K_iMEM		(0x04 << 27)
> +#define CORE_CTRL_PBCLK_ENABLE			BIT(26)
> +#define CORE_CTRL_RUN_STALL			BIT(23)
> +#define CORE_CTRL_STATE_VECTOR_SELECT		BIT(19)
> +#define CORE_CTRL_PIF_GATED			BIT(17)
> +#define CORE_CTRL_NMI				BIT(0)
> +#define CORE_XTENSA_INT				(0x00000114)
> +#define CORE_CTL_XTENSA_INT			(0x00000118)
> +#define CORE_DEFAULT0				(0x0000013C)
> +#define CORE_DEFAULT0_QOS_SWAP_0		(0x00 << 28)
> +#define CORE_DEFAULT0_QOS_SWAP_1		(0x01 << 28)
> +#define CORE_DEFAULT0_QOS_SWAP_2		(0x02 << 28)
> +#define CORE_DEFAULT0_QOS_SWAP_3		(0x03 << 28)
> +#define CORE_DEFAULT0_ARUSER_USE_IOMMU		(0x10 << 23)
> +#define CORE_DEFAULT0_AWUSER_USE_IOMMU		(0x10 << 18)
> +#define CORE_DEFAULT1				(0x00000140)
> +#define CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU	(0x10 << 0)
> +#define CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU	(0x10 << 5)
> +#define CORE_XTENSA_ALTRESETVEC			(0x000001F8)
> +
> +#define RSC_VENDOR_CARVEOUT			(RSC_VENDOR_START + 1)
> +
> +#define APU_RESET_DELAY				(27)
> +
> +struct mtk_apu_rproc {
> +	struct device *dev;
> +	void __iomem *base;
> +	int irq;
> +	struct clk_bulk_data clks[3];
> +	struct list_head mappings;
> +};
> +
> +static int mtk_apu_iommu_map(struct rproc *rproc, struct rproc_mem_entry *entry)
> +{
> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
> +	struct device *dev = rproc->dev.parent;
> +	struct rproc_mem_entry *mapping;
> +	struct iommu_domain *domain;
> +	int ret;
> +	u64 pa;
> +
> +	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
> +	if (!mapping)
> +		return -ENOMEM;
> +
> +	if (!entry->va)
> +		pa = entry->dma;
> +	else
> +		pa = rproc_va_to_pa(entry->va);
> +
> +	if ((strcmp(entry->name, "vdev0vring0") == 0 ||
> +		strcmp(entry->name, "vdev0vring1") == 0)) {

If my guesswork is correct (see below) the above is required to deal with
multiple remote processors.  Why not just look for "vring0" and "vring1"?.  That
way we wouldn't have to keep the "vdev0" part for carveouts that really are
"vdev1". 

> +		entry->va = memremap(entry->dma, entry->len, MEMREMAP_WC);
> +		if (IS_ERR_OR_NULL(entry->va)) {
> +			dev_err(dev, "Unable to map memory region: %pa+%lx\n",
> +				&entry->dma, entry->len);
> +			ret = PTR_ERR(mapping->va);
> +			goto free_mapping;
> +		}
> +		mapping->va = entry->va;
> +	}
> +
> +	domain = iommu_get_domain_for_dev(dev);

Here @domain can be NULL

> +	ret = iommu_map(domain, entry->da, pa, entry->len, entry->flags);

... and iommu_map() isn't dealing with that condition properly.  Please
check for a NULL condition before proceeding.

> +	if (ret) {
> +		dev_err(dev, "iommu_map failed: %d\n", ret);
> +		goto err_memunmap;
> +	}
> +
> +	mapping->da = entry->da;
> +	mapping->len = entry->len;
> +	list_add_tail(&mapping->node, &apu_rproc->mappings);
> +
> +	return 0;
> +
> +err_memunmap:
> +	memunmap(mapping->va);
> +free_mapping:
> +	kfree(mapping);
> +
> +	return ret;
> +}
> +
> +static void mtk_apu_iommu_unmap_all(struct rproc *rproc)
> +{
> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
> +	struct device *dev = rproc->dev.parent;
> +	struct rproc_mem_entry *entry, *tmp;
> +	struct iommu_domain *domain;
> +
> +	/* clean up iommu mapping entries */
> +	list_for_each_entry_safe(entry, tmp, &apu_rproc->mappings, node) {
> +		size_t unmapped;
> +
> +		domain = iommu_get_domain_for_dev(dev);
> +		unmapped = iommu_unmap(domain, entry->da, entry->len);

Same as above

> +		if (unmapped != entry->len) {
> +			/* nothing much to do besides complaining */
> +			dev_err(dev, "failed to unmap %zx/%zu\n", entry->len,
> +				unmapped);
> +		}
> +		memunmap(entry->va);
> +
> +		list_del(&entry->node);
> +		kfree(entry);
> +	}
> +}
> +
> +static int mtk_apu_rproc_prepare(struct rproc *rproc)
> +{
> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(apu_rproc->clks),
> +				      apu_rproc->clks);
> +	if (ret)
> +		dev_err(apu_rproc->dev, "Failed to enable clocks\n");
> +
> +	return ret;
> +}
> +
> +static int mtk_apu_rproc_unprepare(struct rproc *rproc)
> +{
> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
> +
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(apu_rproc->clks),
> +				   apu_rproc->clks);
> +
> +	return 0;
> +}
> +
> +static int mtk_apu_rproc_start(struct rproc *rproc)
> +{
> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
> +	u32 core_ctrl;
> +
> +	/* Set reset vector of APU firmware boot address */
> +	writel(rproc->bootaddr, apu_rproc->base + CORE_XTENSA_ALTRESETVEC);
> +
> +	/* Turn on the clocks and stall the APU */
> +	core_ctrl = readl(apu_rproc->base + CORE_CTRL);
> +	core_ctrl |= CORE_CTRL_PDEBUG_ENABLE | CORE_CTRL_PBCLK_ENABLE |
> +		     CORE_CTRL_STATE_VECTOR_SELECT | CORE_CTRL_RUN_STALL |
> +		     CORE_CTRL_PIF_GATED;
> +	writel(core_ctrl, apu_rproc->base + CORE_CTRL);
> +
> +	/* Reset the APU: this requires 27 ns to be effective on any platform */
> +	writel(SW_RST_OCD_HALT_ON_RST | SW_RST_IPU_B_RST | SW_RST_IPU_D_RST,
> +		apu_rproc->base + SW_RST);
> +	ndelay(APU_RESET_DELAY);
> +	writel(0, apu_rproc->base + SW_RST);
> +
> +	core_ctrl &= ~CORE_CTRL_PIF_GATED;
> +	writel(core_ctrl, apu_rproc->base + CORE_CTRL);
> +
> +	/* Configure memory accesses to go through the IOMMU */
> +	writel(CORE_DEFAULT0_AWUSER_USE_IOMMU | CORE_DEFAULT0_ARUSER_USE_IOMMU |
> +	      CORE_DEFAULT0_QOS_SWAP_1, apu_rproc->base + CORE_DEFAULT0);
> +	writel(CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU |
> +		CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU,
> +		apu_rproc->base + CORE_DEFAULT1);
> +
> +	/* Release the APU */
> +	core_ctrl &= ~CORE_CTRL_RUN_STALL;
> +	writel(core_ctrl, apu_rproc->base + CORE_CTRL);
> +
> +	return 0;
> +}
> +
> +static int mtk_apu_rproc_stop(struct rproc *rproc)
> +{
> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
> +	u32 core_ctrl;
> +
> +	core_ctrl = readl(apu_rproc->base + CORE_CTRL);
> +	writel(core_ctrl | CORE_CTRL_RUN_STALL, apu_rproc->base + CORE_CTRL);
> +
> +	mtk_apu_iommu_unmap_all(rproc);
> +
> +	return 0;
> +}
> +
> +static void mtk_apu_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
> +
> +	writel(1 << vqid, apu_rproc->base + CORE_CTL_XTENSA_INT);
> +}
> +
> +static int mtk_apu_load(struct rproc *rproc, const struct firmware *fw)
> +
> +{
> +	struct rproc_mem_entry *entry, *tmp;
> +	int ret;
> +
> +	ret = rproc_elf_load_segments(rproc, fw);
> +	if (ret)
> +		return ret;
> +
> +	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
> +		ret = mtk_apu_iommu_map(rproc, entry);
> +		if (ret)
> +			goto err_unmap_all;
> +	}
> +
> +	return 0;
> +
> +err_unmap_all:
> +	mtk_apu_iommu_unmap_all(rproc);
> +
> +	return ret;
> +}
> +
> +static struct reserved_mem *of_reserved_mem_by_name(struct rproc *rproc,
> +						    const char *name)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *target;
> +	struct reserved_mem *rmem;
> +	int idx;
> +
> +	idx = of_property_match_string(np, "memory-region-names", name);
> +	if (idx < 0) {
> +		dev_err(dev, "failed to find %s memory\n", name);
> +		return NULL;
> +	}
> +
> +	target = of_parse_phandle(np, "memory-region", idx);
> +	if (!target)
> +		return NULL;

There is no comment to explain the above gymnastic and as such I have to guess
the firmware resources always have "vdev0" prepended to buffer, vring0 and
vring1.  If I am correct please add a comment to describe the situation, ditto
if I am not correct.

> +
> +	rmem = of_reserved_mem_lookup(target);
> +	if (!rmem)
> +		dev_err(dev, "unable to acquire memory-region\n");
> +	of_node_put(target);
> +
> +	return rmem;
> +}
> +
> +static int mtk_apu_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
> +			      int offset, int avail)
> +{
> +	struct device *dev = rproc->dev.parent;
> +
> +	if (rsc_type == RSC_VENDOR_CARVEOUT) {
> +		struct fw_rsc_carveout *rsc_carveout = rsc;
> +		struct rproc_mem_entry *mem;
> +		struct reserved_mem *rmem;
> +
> +		rmem = of_reserved_mem_by_name(rproc, rsc_carveout->name);

s/of_reserved_mem_by_name/mtk_of_reserved_mem_by_name

> +		if (!rmem)
> +			return -ENOMEM;
> +
> +		if (rmem->size < rsc_carveout->len) {

I think '!=' would be more appropriate than '<'.  

> +			dev_err(dev, "The reserved memory is too small\n");
> +			return -ENOMEM;
> +		}
> +
> +		mem = rproc_mem_entry_init(dev, NULL, (dma_addr_t)rmem->base,
> +					   rsc_carveout->len, rsc_carveout->da,
> +					   NULL, NULL, rsc_carveout->name);

Why not naming the carveout the way it is in the DT rather than keeping with
what's in the firmware image?

> +		if (!mem)
> +			return -ENOMEM;
> +
> +		mem->flags = rsc_carveout->flags;
> +		rsc_carveout->pa = rmem->base;
> +		rproc_add_carveout(rproc, mem);
> +	}
> +
> +	return RSC_HANDLED;
> +}
> +
> +static const struct rproc_ops mtk_apu_rproc_ops = {
> +	.prepare		= mtk_apu_rproc_prepare,
> +	.unprepare		= mtk_apu_rproc_unprepare,
> +	.start			= mtk_apu_rproc_start,
> +	.stop			= mtk_apu_rproc_stop,
> +	.kick			= mtk_apu_rproc_kick,
> +	.load			= mtk_apu_load,
> +	.parse_fw		= rproc_elf_load_rsc_table,
> +	.find_loaded_rsc_table	= rproc_elf_find_loaded_rsc_table,
> +	.sanity_check		= rproc_elf_sanity_check,
> +	.get_boot_addr		= rproc_elf_get_boot_addr,
> +	.handle_rsc		= mtk_apu_handle_rsc,
> +};
> +
> +static irqreturn_t mtk_apu_rproc_callback(int irq, void *data)
> +{
> +	struct rproc *rproc = data;
> +	struct mtk_apu_rproc *apu_rproc = (struct mtk_apu_rproc *)rproc->priv;
> +
> +	writel(1, apu_rproc->base + CORE_XTENSA_INT);
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t handle_event(int irq, void *data)
> +{
> +	struct rproc *rproc = data;
> +
> +	rproc_vq_interrupt(rproc, 0);
> +	rproc_vq_interrupt(rproc, 1);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mtk_apu_rproc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_apu_rproc *apu_rproc;
> +	struct rproc *rproc;
> +	struct resource *res;
> +	int ret;
> +
> +	rproc = rproc_alloc(dev, dev_name(dev), &mtk_apu_rproc_ops, NULL,
> +			    sizeof(*apu_rproc));
> +	if (!rproc)
> +		return -ENOMEM;
> +
> +	rproc->recovery_disabled = true;
> +	rproc->has_iommu = false;
> +	rproc->auto_boot = false;
> +
> +	apu_rproc = rproc->priv;
> +	apu_rproc->dev = dev;
> +	INIT_LIST_HEAD(&apu_rproc->mappings);
> +
> +	platform_set_drvdata(pdev, rproc);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	apu_rproc->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(apu_rproc->base)) {
> +		dev_err(dev, "Failed to map mmio\n");
> +		ret = PTR_ERR(apu_rproc->base);
> +		goto free_rproc;
> +	}
> +
> +	apu_rproc->irq = platform_get_irq(pdev, 0);
> +	if (apu_rproc->irq < 0) {
> +		ret = apu_rproc->irq;
> +		goto free_rproc;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, apu_rproc->irq,
> +					mtk_apu_rproc_callback, handle_event,
> +					IRQF_SHARED | IRQF_ONESHOT,
> +					NULL, rproc);

I'm pretty sure I commented on that during my last review - is there a need to
do a threaded irq here?  As far as I can tell there is no advantage in doing so
in this context.  Please add a comment to justify using a threaded irq.

Thanks,
Mathieu

> +	if (ret) {
> +		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
> +		goto free_rproc;
> +	}
> +
> +	apu_rproc->clks[0].id = "ipu";
> +	apu_rproc->clks[1].id = "axi";
> +	apu_rproc->clks[2].id = "jtag";
> +
> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(apu_rproc->clks),
> +				apu_rproc->clks);
> +	if (ret) {
> +		dev_err(dev, "Failed to get clocks\n");
> +		goto free_rproc;
> +	}
> +
> +	ret = rproc_add(rproc);
> +	if (ret) {
> +		dev_err(dev, "rproc_add failed: %d\n", ret);
> +		goto free_rproc;
> +	}
> +
> +	return 0;
> +
> +free_rproc:
> +	rproc_free(rproc);
> +
> +	return ret;
> +}
> +
> +static int mtk_apu_rproc_remove(struct platform_device *pdev)
> +{
> +	struct rproc *rproc = platform_get_drvdata(pdev);
> +	struct mtk_apu_rproc *apu_rproc = (struct mtk_apu_rproc *)rproc->priv;
> +	struct device *dev = &pdev->dev;
> +
> +	disable_irq(apu_rproc->irq);
> +
> +	rproc_del(rproc);
> +	of_reserved_mem_device_release(dev);
> +	rproc_free(rproc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mtk_apu_rproc_of_match[] = {
> +	{ .compatible = "mediatek,mt8183-apu", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_apu_rproc_of_match);
> +
> +static struct platform_driver mtk_apu_rproc_driver = {
> +	.probe = mtk_apu_rproc_probe,
> +	.remove = mtk_apu_rproc_remove,
> +	.driver = {
> +		.name = "mtk_apu-rproc",
> +		.of_match_table = of_match_ptr(mtk_apu_rproc_of_match),
> +	},
> +};
> +module_platform_driver(mtk_apu_rproc_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Alexandre Bailon");
> +MODULE_DESCRIPTION("MTK APU Remote Processor control driver");
> -- 
> 2.31.1
> 

Re: [PATCH v3 2/4] remoteproc: Add a remoteproc driver for the MT8183's APU

[Alexandre Bailon]

Hi Mathieu,

Le 30/08/2021 à 20:19, Mathieu Poirier a écrit :
> Hi Alex,
>
> On Thu, Aug 19, 2021 at 05:13:38PM +0200, Alexandre Bailon wrote:
>> This adds a driver to control the APU present in the MT8183.
>> This loads the firmware and start the DSP.
>>
>> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
>> ---
>>   drivers/remoteproc/Kconfig   |  10 +
>>   drivers/remoteproc/Makefile  |   1 +
>>   drivers/remoteproc/mtk_apu.c | 440 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 451 insertions(+)
>>   create mode 100644 drivers/remoteproc/mtk_apu.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index 9a6eedc3994a5..6c106a6c3ad5d 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -53,6 +53,16 @@ config MTK_SCP
>>   
>>   	  It's safe to say N here.
>>   
>> +config MTK_APU
>> +	tristate "Mediatek APU remoteproc support"
>> +	depends on ARCH_MEDIATEK
>> +	depends on MTK_IOMMU
>> +	help
>> +	  Say y to support the Mediatek's Accelerated Processing Unit (APU) via
> In the bindings A in APU is for AI or Vision whereas above it is for
> accelerated.  Please pick one and stick with it.
Indeed, the A stands for AI so I missed that one.
>> +	  the remote processor framework.
>> +
>> +	  It's safe to say N here.
>> +
>>   config OMAP_REMOTEPROC
>>   	tristate "OMAP remoteproc support"
>>   	depends on ARCH_OMAP4 || SOC_OMAP5 || SOC_DRA7XX
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index bb26c9e4ef9cf..e395af86c17b0 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.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_MTK_APU)			+= mtk_apu.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/mtk_apu.c b/drivers/remoteproc/mtk_apu.c
>> new file mode 100644
>> index 0000000000000..0e3f63987bd85
>> --- /dev/null
>> +++ b/drivers/remoteproc/mtk_apu.c
>> @@ -0,0 +1,440 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2020 BayLibre SAS
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iommu.h>
>> +#include <linux/irq.h>
>> +#include <linux/module.h>
>> +#include <linux/of_reserved_mem.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc.h>
>> +
>> +#include "remoteproc_internal.h"
>> +
>> +#define SW_RST					(0x0000000C)
>> +#define SW_RST_OCD_HALT_ON_RST			BIT(12)
>> +#define SW_RST_IPU_D_RST			BIT(8)
>> +#define SW_RST_IPU_B_RST			BIT(4)
>> +#define CORE_CTRL				(0x00000110)
>> +#define CORE_CTRL_PDEBUG_ENABLE			BIT(31)
>> +#define CORE_CTRL_SRAM_64K_iMEM			(0x00 << 27)
>> +#define CORE_CTRL_SRAM_96K_iMEM			(0x01 << 27)
>> +#define CORE_CTRL_SRAM_128K_iMEM		(0x02 << 27)
>> +#define CORE_CTRL_SRAM_192K_iMEM		(0x03 << 27)
>> +#define CORE_CTRL_SRAM_256K_iMEM		(0x04 << 27)
>> +#define CORE_CTRL_PBCLK_ENABLE			BIT(26)
>> +#define CORE_CTRL_RUN_STALL			BIT(23)
>> +#define CORE_CTRL_STATE_VECTOR_SELECT		BIT(19)
>> +#define CORE_CTRL_PIF_GATED			BIT(17)
>> +#define CORE_CTRL_NMI				BIT(0)
>> +#define CORE_XTENSA_INT				(0x00000114)
>> +#define CORE_CTL_XTENSA_INT			(0x00000118)
>> +#define CORE_DEFAULT0				(0x0000013C)
>> +#define CORE_DEFAULT0_QOS_SWAP_0		(0x00 << 28)
>> +#define CORE_DEFAULT0_QOS_SWAP_1		(0x01 << 28)
>> +#define CORE_DEFAULT0_QOS_SWAP_2		(0x02 << 28)
>> +#define CORE_DEFAULT0_QOS_SWAP_3		(0x03 << 28)
>> +#define CORE_DEFAULT0_ARUSER_USE_IOMMU		(0x10 << 23)
>> +#define CORE_DEFAULT0_AWUSER_USE_IOMMU		(0x10 << 18)
>> +#define CORE_DEFAULT1				(0x00000140)
>> +#define CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU	(0x10 << 0)
>> +#define CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU	(0x10 << 5)
>> +#define CORE_XTENSA_ALTRESETVEC			(0x000001F8)
>> +
>> +#define RSC_VENDOR_CARVEOUT			(RSC_VENDOR_START + 1)
>> +
>> +#define APU_RESET_DELAY				(27)
>> +
>> +struct mtk_apu_rproc {
>> +	struct device *dev;
>> +	void __iomem *base;
>> +	int irq;
>> +	struct clk_bulk_data clks[3];
>> +	struct list_head mappings;
>> +};
>> +
>> +static int mtk_apu_iommu_map(struct rproc *rproc, struct rproc_mem_entry *entry)
>> +{
>> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
>> +	struct device *dev = rproc->dev.parent;
>> +	struct rproc_mem_entry *mapping;
>> +	struct iommu_domain *domain;
>> +	int ret;
>> +	u64 pa;
>> +
>> +	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
>> +	if (!mapping)
>> +		return -ENOMEM;
>> +
>> +	if (!entry->va)
>> +		pa = entry->dma;
>> +	else
>> +		pa = rproc_va_to_pa(entry->va);
>> +
>> +	if ((strcmp(entry->name, "vdev0vring0") == 0 ||
>> +		strcmp(entry->name, "vdev0vring1") == 0)) {
> If my guesswork is correct (see below) the above is required to deal with
> multiple remote processors.  Why not just look for "vring0" and "vring1"?.  That
> way we wouldn't have to keep the "vdev0" part for carveouts that really are
> "vdev1".
This makes sense. I will do that change.
>
>> +		entry->va = memremap(entry->dma, entry->len, MEMREMAP_WC);
>> +		if (IS_ERR_OR_NULL(entry->va)) {
>> +			dev_err(dev, "Unable to map memory region: %pa+%lx\n",
>> +				&entry->dma, entry->len);
>> +			ret = PTR_ERR(mapping->va);
>> +			goto free_mapping;
>> +		}
>> +		mapping->va = entry->va;
>> +	}
>> +
>> +	domain = iommu_get_domain_for_dev(dev);
> Here @domain can be NULL
>
>> +	ret = iommu_map(domain, entry->da, pa, entry->len, entry->flags);
> ... and iommu_map() isn't dealing with that condition properly.  Please
> check for a NULL condition before proceeding.
>
>> +	if (ret) {
>> +		dev_err(dev, "iommu_map failed: %d\n", ret);
>> +		goto err_memunmap;
>> +	}
>> +
>> +	mapping->da = entry->da;
>> +	mapping->len = entry->len;
>> +	list_add_tail(&mapping->node, &apu_rproc->mappings);
>> +
>> +	return 0;
>> +
>> +err_memunmap:
>> +	memunmap(mapping->va);
>> +free_mapping:
>> +	kfree(mapping);
>> +
>> +	return ret;
>> +}
>> +
>> +static void mtk_apu_iommu_unmap_all(struct rproc *rproc)
>> +{
>> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
>> +	struct device *dev = rproc->dev.parent;
>> +	struct rproc_mem_entry *entry, *tmp;
>> +	struct iommu_domain *domain;
>> +
>> +	/* clean up iommu mapping entries */
>> +	list_for_each_entry_safe(entry, tmp, &apu_rproc->mappings, node) {
>> +		size_t unmapped;
>> +
>> +		domain = iommu_get_domain_for_dev(dev);
>> +		unmapped = iommu_unmap(domain, entry->da, entry->len);
> Same as above
>
>> +		if (unmapped != entry->len) {
>> +			/* nothing much to do besides complaining */
>> +			dev_err(dev, "failed to unmap %zx/%zu\n", entry->len,
>> +				unmapped);
>> +		}
>> +		memunmap(entry->va);
>> +
>> +		list_del(&entry->node);
>> +		kfree(entry);
>> +	}
>> +}
>> +
>> +static int mtk_apu_rproc_prepare(struct rproc *rproc)
>> +{
>> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
>> +	int ret;
>> +
>> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(apu_rproc->clks),
>> +				      apu_rproc->clks);
>> +	if (ret)
>> +		dev_err(apu_rproc->dev, "Failed to enable clocks\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int mtk_apu_rproc_unprepare(struct rproc *rproc)
>> +{
>> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
>> +
>> +	clk_bulk_disable_unprepare(ARRAY_SIZE(apu_rproc->clks),
>> +				   apu_rproc->clks);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mtk_apu_rproc_start(struct rproc *rproc)
>> +{
>> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
>> +	u32 core_ctrl;
>> +
>> +	/* Set reset vector of APU firmware boot address */
>> +	writel(rproc->bootaddr, apu_rproc->base + CORE_XTENSA_ALTRESETVEC);
>> +
>> +	/* Turn on the clocks and stall the APU */
>> +	core_ctrl = readl(apu_rproc->base + CORE_CTRL);
>> +	core_ctrl |= CORE_CTRL_PDEBUG_ENABLE | CORE_CTRL_PBCLK_ENABLE |
>> +		     CORE_CTRL_STATE_VECTOR_SELECT | CORE_CTRL_RUN_STALL |
>> +		     CORE_CTRL_PIF_GATED;
>> +	writel(core_ctrl, apu_rproc->base + CORE_CTRL);
>> +
>> +	/* Reset the APU: this requires 27 ns to be effective on any platform */
>> +	writel(SW_RST_OCD_HALT_ON_RST | SW_RST_IPU_B_RST | SW_RST_IPU_D_RST,
>> +		apu_rproc->base + SW_RST);
>> +	ndelay(APU_RESET_DELAY);
>> +	writel(0, apu_rproc->base + SW_RST);
>> +
>> +	core_ctrl &= ~CORE_CTRL_PIF_GATED;
>> +	writel(core_ctrl, apu_rproc->base + CORE_CTRL);
>> +
>> +	/* Configure memory accesses to go through the IOMMU */
>> +	writel(CORE_DEFAULT0_AWUSER_USE_IOMMU | CORE_DEFAULT0_ARUSER_USE_IOMMU |
>> +	      CORE_DEFAULT0_QOS_SWAP_1, apu_rproc->base + CORE_DEFAULT0);
>> +	writel(CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU |
>> +		CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU,
>> +		apu_rproc->base + CORE_DEFAULT1);
>> +
>> +	/* Release the APU */
>> +	core_ctrl &= ~CORE_CTRL_RUN_STALL;
>> +	writel(core_ctrl, apu_rproc->base + CORE_CTRL);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mtk_apu_rproc_stop(struct rproc *rproc)
>> +{
>> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
>> +	u32 core_ctrl;
>> +
>> +	core_ctrl = readl(apu_rproc->base + CORE_CTRL);
>> +	writel(core_ctrl | CORE_CTRL_RUN_STALL, apu_rproc->base + CORE_CTRL);
>> +
>> +	mtk_apu_iommu_unmap_all(rproc);
>> +
>> +	return 0;
>> +}
>> +
>> +static void mtk_apu_rproc_kick(struct rproc *rproc, int vqid)
>> +{
>> +	struct mtk_apu_rproc *apu_rproc = rproc->priv;
>> +
>> +	writel(1 << vqid, apu_rproc->base + CORE_CTL_XTENSA_INT);
>> +}
>> +
>> +static int mtk_apu_load(struct rproc *rproc, const struct firmware *fw)
>> +
>> +{
>> +	struct rproc_mem_entry *entry, *tmp;
>> +	int ret;
>> +
>> +	ret = rproc_elf_load_segments(rproc, fw);
>> +	if (ret)
>> +		return ret;
>> +
>> +	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
>> +		ret = mtk_apu_iommu_map(rproc, entry);
>> +		if (ret)
>> +			goto err_unmap_all;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_unmap_all:
>> +	mtk_apu_iommu_unmap_all(rproc);
>> +
>> +	return ret;
>> +}
>> +
>> +static struct reserved_mem *of_reserved_mem_by_name(struct rproc *rproc,
>> +						    const char *name)
>> +{
>> +	struct device *dev = rproc->dev.parent;
>> +	struct device_node *np = dev->of_node;
>> +	struct device_node *target;
>> +	struct reserved_mem *rmem;
>> +	int idx;
>> +
>> +	idx = of_property_match_string(np, "memory-region-names", name);
>> +	if (idx < 0) {
>> +		dev_err(dev, "failed to find %s memory\n", name);
>> +		return NULL;
>> +	}
>> +
>> +	target = of_parse_phandle(np, "memory-region", idx);
>> +	if (!target)
>> +		return NULL;
> There is no comment to explain the above gymnastic and as such I have to guess
> the firmware resources always have "vdev0" prepended to buffer, vring0 and
> vring1.  If I am correct please add a comment to describe the situation, ditto
> if I am not correct.
OK, I will add much more documentation to explain.
>
>> +
>> +	rmem = of_reserved_mem_lookup(target);
>> +	if (!rmem)
>> +		dev_err(dev, "unable to acquire memory-region\n");
>> +	of_node_put(target);
>> +
>> +	return rmem;
>> +}
>> +
>> +static int mtk_apu_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
>> +			      int offset, int avail)
>> +{
>> +	struct device *dev = rproc->dev.parent;
>> +
>> +	if (rsc_type == RSC_VENDOR_CARVEOUT) {
>> +		struct fw_rsc_carveout *rsc_carveout = rsc;
>> +		struct rproc_mem_entry *mem;
>> +		struct reserved_mem *rmem;
>> +
>> +		rmem = of_reserved_mem_by_name(rproc, rsc_carveout->name);
> s/of_reserved_mem_by_name/mtk_of_reserved_mem_by_name
>
>> +		if (!rmem)
>> +			return -ENOMEM;
>> +
>> +		if (rmem->size < rsc_carveout->len) {
> I think '!=' would be more appropriate than '<'.
>
>> +			dev_err(dev, "The reserved memory is too small\n");
>> +			return -ENOMEM;
>> +		}
>> +
>> +		mem = rproc_mem_entry_init(dev, NULL, (dma_addr_t)rmem->base,
>> +					   rsc_carveout->len, rsc_carveout->da,
>> +					   NULL, NULL, rsc_carveout->name);
> Why not naming the carveout the way it is in the DT rather than keeping with
> what's in the firmware image?
The main reason is that we may load different firmware at runtime, with 
very different memory layouts.
Some firmware only need few MB of memory whereas some other may requires 
about 64 MB of memory.
Using the resource table here gives much more flexibility than DT that 
is expected to be "static".
>
>> +		if (!mem)
>> +			return -ENOMEM;
>> +
>> +		mem->flags = rsc_carveout->flags;
>> +		rsc_carveout->pa = rmem->base;
>> +		rproc_add_carveout(rproc, mem);
>> +	}
>> +
>> +	return RSC_HANDLED;
>> +}
>> +
>> +static const struct rproc_ops mtk_apu_rproc_ops = {
>> +	.prepare		= mtk_apu_rproc_prepare,
>> +	.unprepare		= mtk_apu_rproc_unprepare,
>> +	.start			= mtk_apu_rproc_start,
>> +	.stop			= mtk_apu_rproc_stop,
>> +	.kick			= mtk_apu_rproc_kick,
>> +	.load			= mtk_apu_load,
>> +	.parse_fw		= rproc_elf_load_rsc_table,
>> +	.find_loaded_rsc_table	= rproc_elf_find_loaded_rsc_table,
>> +	.sanity_check		= rproc_elf_sanity_check,
>> +	.get_boot_addr		= rproc_elf_get_boot_addr,
>> +	.handle_rsc		= mtk_apu_handle_rsc,
>> +};
>> +
>> +static irqreturn_t mtk_apu_rproc_callback(int irq, void *data)
>> +{
>> +	struct rproc *rproc = data;
>> +	struct mtk_apu_rproc *apu_rproc = (struct mtk_apu_rproc *)rproc->priv;
>> +
>> +	writel(1, apu_rproc->base + CORE_XTENSA_INT);
>> +
>> +	return IRQ_WAKE_THREAD;
>> +}
>> +
>> +static irqreturn_t handle_event(int irq, void *data)
>> +{
>> +	struct rproc *rproc = data;
>> +
>> +	rproc_vq_interrupt(rproc, 0);
>> +	rproc_vq_interrupt(rproc, 1);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int mtk_apu_rproc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct mtk_apu_rproc *apu_rproc;
>> +	struct rproc *rproc;
>> +	struct resource *res;
>> +	int ret;
>> +
>> +	rproc = rproc_alloc(dev, dev_name(dev), &mtk_apu_rproc_ops, NULL,
>> +			    sizeof(*apu_rproc));
>> +	if (!rproc)
>> +		return -ENOMEM;
>> +
>> +	rproc->recovery_disabled = true;
>> +	rproc->has_iommu = false;
>> +	rproc->auto_boot = false;
>> +
>> +	apu_rproc = rproc->priv;
>> +	apu_rproc->dev = dev;
>> +	INIT_LIST_HEAD(&apu_rproc->mappings);
>> +
>> +	platform_set_drvdata(pdev, rproc);
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	apu_rproc->base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(apu_rproc->base)) {
>> +		dev_err(dev, "Failed to map mmio\n");
>> +		ret = PTR_ERR(apu_rproc->base);
>> +		goto free_rproc;
>> +	}
>> +
>> +	apu_rproc->irq = platform_get_irq(pdev, 0);
>> +	if (apu_rproc->irq < 0) {
>> +		ret = apu_rproc->irq;
>> +		goto free_rproc;
>> +	}
>> +
>> +	ret = devm_request_threaded_irq(dev, apu_rproc->irq,
>> +					mtk_apu_rproc_callback, handle_event,
>> +					IRQF_SHARED | IRQF_ONESHOT,
>> +					NULL, rproc);
> I'm pretty sure I commented on that during my last review - is there a need to
> do a threaded irq here?  As far as I can tell there is no advantage in doing so
> in this context.  Please add a comment to justify using a threaded irq.
My apologize if I forgot to address a comment you made.
It used to be required before I moved to virtio / rpmsg to manage the 
communication before
the CPU and the APU (the driver was doing a lot of thing in the callback).
Now, with the latest architecture, I am not sure if that is still 
required since the callback is just
going through a list of request, copy rpmsg data and then notify DRM 
that a request has been received.

Thanks,
Alexandre
>
> Thanks,
> Mathieu
>
>> +	if (ret) {
>> +		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
>> +		goto free_rproc;
>> +	}
>> +
>> +	apu_rproc->clks[0].id = "ipu";
>> +	apu_rproc->clks[1].id = "axi";
>> +	apu_rproc->clks[2].id = "jtag";
>> +
>> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(apu_rproc->clks),
>> +				apu_rproc->clks);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to get clocks\n");
>> +		goto free_rproc;
>> +	}
>> +
>> +	ret = rproc_add(rproc);
>> +	if (ret) {
>> +		dev_err(dev, "rproc_add failed: %d\n", ret);
>> +		goto free_rproc;
>> +	}
>> +
>> +	return 0;
>> +
>> +free_rproc:
>> +	rproc_free(rproc);
>> +
>> +	return ret;
>> +}
>> +
>> +static int mtk_apu_rproc_remove(struct platform_device *pdev)
>> +{
>> +	struct rproc *rproc = platform_get_drvdata(pdev);
>> +	struct mtk_apu_rproc *apu_rproc = (struct mtk_apu_rproc *)rproc->priv;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	disable_irq(apu_rproc->irq);
>> +
>> +	rproc_del(rproc);
>> +	of_reserved_mem_device_release(dev);
>> +	rproc_free(rproc);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id mtk_apu_rproc_of_match[] = {
>> +	{ .compatible = "mediatek,mt8183-apu", },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, mtk_apu_rproc_of_match);
>> +
>> +static struct platform_driver mtk_apu_rproc_driver = {
>> +	.probe = mtk_apu_rproc_probe,
>> +	.remove = mtk_apu_rproc_remove,
>> +	.driver = {
>> +		.name = "mtk_apu-rproc",
>> +		.of_match_table = of_match_ptr(mtk_apu_rproc_of_match),
>> +	},
>> +};
>> +module_platform_driver(mtk_apu_rproc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Alexandre Bailon");
>> +MODULE_DESCRIPTION("MTK APU Remote Processor control driver");
>> -- 
>> 2.31.1
>>