[PATCH 0/6] Add support of mt8183 APU

[PATCH 0/6] 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 series depends on two other series:
- Mediatek MT8183 scpsys support  
- arm64: dts: Add m4u and smi-larbs nodes for mt8183 

Notes:
This series include two workarounds:
- remoteproc: mtk_vpu_rproc: Don't try to load empty PT_LOAD segment
- rproc: mtk_apu: Don't try to use local APU RAM

The first one is required to load malformed firmwares.
This is probably caused by the toolchain we are using (a fork of gcc 4.2).
It would be better to fix the firmwares but I don't know how to fix it.

The second one prevents the CPU to access to the APU local RAM.
If the CPU tries to read or write the APU local RAM, then the CPU will
hang. I'm still looking for a solution, but until, we must prevent
remoteproc to write something (usually, to initialize data section).
Because of that issue, the current driver doesn't map the the local RAM.

Alexandre Bailon (6):
  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
  remoteproc: mtk_vpu_rproc: Don't try to load empty PT_LOAD segment
  remoteproc: mtk_apu: Don't try to use the APU local RAM
  ARM64: mt8183: Add support of APU to mt8183

 .../bindings/remoteproc/mtk,apu.yaml          | 121 +++++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      |  42 ++
 drivers/remoteproc/Kconfig                    |  19 +
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/mtk_apu_rproc.c            | 501 ++++++++++++++++++
 5 files changed, 684 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
 create mode 100644 drivers/remoteproc/mtk_apu_rproc.c

-- 
2.26.2

[PATCH 1/6] 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          | 121 ++++++++++++++++++
 1 file changed, 121 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 000000000000..1d5fcc135617
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
@@ -0,0 +1,121 @@
+# 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 Vision Processor Unit (VPU) devices
+
+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:
+    description:
+      Address ranges of the APU MMIO.
+    maxItems: 1
+
+  interrupts:
+    description:
+      The interrupt number used to receive the interrupts from the DSP.
+    maxItems: 1
+
+  clocks:
+    description:
+      "Clocks for co-processor (See: ../clock/clock-bindings.txt)
+      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:
+    description:
+      The name of clocks, must be ipu, axi and jtag
+    items:
+      - const: ipu
+      - const: axi
+      - const: jtag
+
+  iommu:
+    description: "IOMMU (See: ../iommu/iommu.txt)"
+    maxItems: 3
+
+  memory-region:
+    description: "Reserved memory (See: ../reserved-memory/reserved-memory.txt)"
+    maxItems: 1
+
+  power-domains:
+    description: "Power domain (see: ../power/power_domain.txt)"
+    maxItems: 1
+
+  pinctrl:
+    description: pinctrl handles, required to configure pins for JTAG.
+    maxItems: 2
+
+  pinctrl-names:
+    description:
+      pinctrl name, must be "default", "jtag".
+      "default" must configure the pins when JTAG is disabled.
+      "jtag" must configure the pins for JTAG operations.
+    items:
+      - const: default
+      - const: jtag
+    maxItems: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - iommu
+  - memory-region
+  - power-domains
+
+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;
+
+      vpu_ram: vpu_ram@0x60000000 {
+        compatible = "shared-dma-pool";
+        reg = <0 0x60000000 0 0x040000000>;
+        no-map;
+        linux,cma-default;
+      };
+    };
+
+    vpu0: vpu@0x19100000 {
+      compatible = "mediatek,mt8183-apu";
+      reg = <0 0x19180000 0 0x14000>;
+      reg-names = "mmio";
+      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 = <&vpu_ram>;
+    };
+...
-- 
2.26.2

[PATCH 2/6] 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_rproc.c | 308 +++++++++++++++++++++++++++++
 3 files changed, 319 insertions(+)
 create mode 100644 drivers/remoteproc/mtk_apu_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index c4d1731295eb..e116d4a12ac3 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -42,6 +42,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 e8b886e511f0..2ea231b75fa6 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -12,6 +12,7 @@ remoteproc-y				+= remoteproc_elf_loader.o
 obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
 obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
 obj-$(CONFIG_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
+obj-$(CONFIG_MTK_APU)			+= mtk_apu_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/mtk_apu_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
new file mode 100644
index 000000000000..fb416a817ef3
--- /dev/null
+++ b/drivers/remoteproc/mtk_apu_rproc.c
@@ -0,0 +1,308 @@
+// 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/io.h>
+#include <linux/iommu.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/highmem.h>
+#include <linux/module.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+
+#include "remoteproc_internal.h"
+
+/* From MT8183 4.5 Vision Processor Unit (VPU).pdf datasheet */
+#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)
+
+struct mtk_vpu_rproc {
+	struct device *dev;
+	struct rproc *rproc;
+
+	void __iomem *base;
+	int irq;
+	struct clk *axi;
+	struct clk *ipu;
+	struct clk *jtag;
+};
+
+static u32 vpu_read32(struct mtk_vpu_rproc *vpu_rproc, u32 off)
+{
+	return readl(vpu_rproc->base + off);
+}
+
+static void vpu_write32(struct mtk_vpu_rproc *vpu_rproc, u32 off, u32 value)
+{
+	writel(value, vpu_rproc->base + off);
+}
+
+static int mtk_vpu_rproc_start(struct rproc *rproc)
+{
+	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
+	u32 core_ctrl;
+
+	vpu_write32(vpu_rproc, CORE_XTENSA_ALTRESETVEC, rproc->bootaddr);
+
+	core_ctrl = vpu_read32(vpu_rproc, 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;
+	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
+
+	vpu_write32(vpu_rproc, SW_RST, SW_RST_OCD_HALT_ON_RST |
+				       SW_RST_IPU_B_RST | SW_RST_IPU_D_RST);
+	ndelay(27);
+	vpu_write32(vpu_rproc, SW_RST, 0);
+
+	core_ctrl &= ~CORE_CTRL_PIF_GATED;
+	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
+
+	vpu_write32(vpu_rproc, CORE_DEFAULT0, CORE_DEFAULT0_AWUSER_USE_IOMMU |
+					      CORE_DEFAULT0_ARUSER_USE_IOMMU |
+					      CORE_DEFAULT0_QOS_SWAP_1);
+	vpu_write32(vpu_rproc, CORE_DEFAULT1,
+		    CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU |
+		    CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU);
+
+	core_ctrl &= ~CORE_CTRL_RUN_STALL;
+	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
+
+	return 0;
+}
+
+static int mtk_vpu_rproc_stop(struct rproc *rproc)
+{
+	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
+	u32 core_ctrl;
+
+	core_ctrl = vpu_read32(vpu_rproc, CORE_CTRL);
+	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl | CORE_CTRL_RUN_STALL);
+
+	return 0;
+}
+
+static void mtk_vpu_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
+
+	vpu_write32(vpu_rproc, CORE_CTL_XTENSA_INT, 1 << vqid);
+}
+
+static const struct rproc_ops mtk_vpu_rproc_ops = {
+	.start		= mtk_vpu_rproc_start,
+	.stop		= mtk_vpu_rproc_stop,
+	.kick		= mtk_vpu_rproc_kick,
+};
+
+static irqreturn_t mtk_vpu_rproc_callback(int irq, void *data)
+{
+	struct rproc *rproc = (struct rproc *)data;
+	struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
+
+	vpu_write32(vpu_rproc, CORE_XTENSA_INT, 1);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t handle_event(int irq, void *data)
+{
+	struct rproc *rproc = (struct rproc *)data;
+
+	rproc_vq_interrupt(rproc, 0);
+	rproc_vq_interrupt(rproc, 1);
+
+	return IRQ_HANDLED;
+}
+
+static int mtk_vpu_rproc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mtk_vpu_rproc *vpu_rproc;
+	struct rproc *rproc;
+	struct resource *res;
+	int ret;
+
+	rproc = rproc_alloc(dev, "apu", &mtk_vpu_rproc_ops, NULL,
+			    sizeof(*vpu_rproc));
+	if (!rproc)
+		return -ENOMEM;
+
+	rproc->recovery_disabled = true;
+	rproc->has_iommu = false;
+
+	vpu_rproc = rproc->priv;
+	vpu_rproc->rproc = rproc;
+	vpu_rproc->dev = dev;
+
+	platform_set_drvdata(pdev, rproc);
+
+	rproc->domain = iommu_get_domain_for_dev(dev);
+	if (!rproc->domain) {
+		dev_err(dev, "Failed to get the IOMMU domain\n");
+		ret = -EINVAL;
+		goto free_rproc;
+	}
+
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	vpu_rproc->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(vpu_rproc->base)) {
+		dev_err(&pdev->dev, "Failed to map mmio\n");
+		ret = PTR_ERR(vpu_rproc->base);
+		goto free_rproc;
+	}
+
+	vpu_rproc->irq = platform_get_irq(pdev, 0);
+	if (vpu_rproc->irq < 0) {
+		ret = vpu_rproc->irq;
+		goto free_rproc;
+	}
+
+	ret = devm_request_threaded_irq(dev, vpu_rproc->irq,
+					mtk_vpu_rproc_callback, handle_event,
+					IRQF_SHARED | IRQF_ONESHOT,
+					"mtk_vpu-remoteproc", rproc);
+	if (ret) {
+		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
+		goto free_rproc;
+	}
+
+	vpu_rproc->ipu = devm_clk_get(dev, "ipu");
+	if (IS_ERR(vpu_rproc->ipu)) {
+		dev_err(dev, "Failed to get ipu clock\n");
+		ret = PTR_ERR(vpu_rproc->ipu);
+		goto free_rproc;
+	}
+
+	ret = clk_prepare_enable(vpu_rproc->ipu);
+	if (ret) {
+		dev_err(dev, "Failed to enable ipu clock\n");
+		goto free_rproc;
+	}
+
+	vpu_rproc->axi = devm_clk_get(dev, "axi");
+	if (IS_ERR(vpu_rproc->axi)) {
+		dev_err(dev, "Failed to get axi clock\n");
+		ret = PTR_ERR(vpu_rproc->axi);
+		goto clk_disable_ipu;
+	}
+
+	ret = clk_prepare_enable(vpu_rproc->axi);
+	if (ret) {
+		dev_err(dev, "Failed to enable axi clock\n");
+		goto clk_disable_ipu;
+	}
+
+	vpu_rproc->jtag = devm_clk_get_optional(dev, "jtag");
+	if (IS_ERR(vpu_rproc->jtag)) {
+		dev_err(dev, "Failed to enable jtag clock\n");
+		ret = PTR_ERR(vpu_rproc->jtag);
+		goto clk_disable_axi;
+	}
+
+	ret = clk_prepare_enable(vpu_rproc->jtag);
+	if (ret) {
+		dev_err(dev, "Failed to enable jtag clock\n");
+		goto clk_disable_axi;
+	}
+
+	ret = of_reserved_mem_device_init(dev);
+	if (ret) {
+		dev_err(dev, "device does not have specific CMA pool\n");
+		goto clk_disable_jtag;
+	}
+
+	ret = rproc_add(rproc);
+	if (ret) {
+		dev_err(dev, "rproc_add failed: %d\n", ret);
+		goto free_mem;
+	}
+
+	return 0;
+
+free_mem:
+	of_reserved_mem_device_release(dev);
+clk_disable_jtag:
+	clk_disable_unprepare(vpu_rproc->jtag);
+clk_disable_axi:
+	clk_disable_unprepare(vpu_rproc->axi);
+clk_disable_ipu:
+	clk_disable_unprepare(vpu_rproc->ipu);
+free_rproc:
+	rproc_free(rproc);
+
+	return ret;
+}
+
+static int mtk_vpu_rproc_remove(struct platform_device *pdev)
+{
+	struct rproc *rproc = platform_get_drvdata(pdev);
+	struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
+	struct device *dev = &pdev->dev;
+
+	disable_irq(vpu_rproc->irq);
+
+	rproc_del(rproc);
+	of_reserved_mem_device_release(dev);
+	clk_disable_unprepare(vpu_rproc->jtag);
+	clk_disable_unprepare(vpu_rproc->axi);
+	clk_disable_unprepare(vpu_rproc->ipu);
+	rproc_free(rproc);
+
+	return 0;
+}
+
+static const struct of_device_id mtk_vpu_rproc_of_match[] __maybe_unused = {
+	{ .compatible = "mediatek,mt8183-apu", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mtk_vpu_rproc_of_match);
+
+static struct platform_driver mtk_vpu_rproc_driver = {
+	.probe = mtk_vpu_rproc_probe,
+	.remove = mtk_vpu_rproc_remove,
+	.driver = {
+		.name = "mtk_vpu-rproc",
+		.of_match_table = of_match_ptr(mtk_vpu_rproc_of_match),
+	},
+};
+module_platform_driver(mtk_vpu_rproc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Alexandre Bailon");
+MODULE_DESCRIPTION("Mt8183 VPU Remote Processor control driver");
-- 
2.26.2

[PATCH 3/6] 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_rproc.c | 156 ++++++++++++++++++++++++++++-
 2 files changed, 162 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index e116d4a12ac3..e1158563e2e8 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -52,6 +52,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_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
index fb416a817ef3..f2342b747a35 100644
--- a/drivers/remoteproc/mtk_apu_rproc.c
+++ b/drivers/remoteproc/mtk_apu_rproc.c
@@ -5,6 +5,7 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -14,6 +15,7 @@
 #include <linux/highmem.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>
 
@@ -48,6 +50,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)
 
 struct mtk_vpu_rproc {
@@ -59,6 +66,13 @@ struct mtk_vpu_rproc {
 	struct clk *axi;
 	struct clk *ipu;
 	struct clk *jtag;
+
+#ifdef CONFIG_MTK_APU_JTAG
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pinctrl_default;
+	struct pinctrl_state *pinctrl_jtag;
+	bool jtag_enabled;
+#endif
 };
 
 static u32 vpu_read32(struct mtk_vpu_rproc *vpu_rproc, u32 off)
@@ -149,6 +163,133 @@ static irqreturn_t handle_event(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_MTK_APU_JTAG
+
+static int vpu_enable_jtag(struct mtk_vpu_rproc *vpu_rproc)
+{
+	int ret = 0;
+
+	if (vpu_rproc->jtag_enabled)
+		return -EINVAL;
+
+	ret = pinctrl_select_state(vpu_rproc->pinctrl,
+				   vpu_rproc->pinctrl_jtag);
+	if (ret < 0) {
+		dev_err(vpu_rproc->dev, "Failed to configure pins for JTAG\n");
+		return ret;
+	}
+
+	vpu_write32(vpu_rproc, CORE_DEFAULT2,
+		    CORE_DEFAULT2_SPNIDEN | CORE_DEFAULT2_SPIDEN |
+		    CORE_DEFAULT2_NIDEN | CORE_DEFAULT2_DBG_EN);
+
+	vpu_rproc->jtag_enabled = 1;
+
+	return ret;
+}
+
+static int vpu_disable_jtag(struct mtk_vpu_rproc *vpu_rproc)
+{
+	int ret = 0;
+
+	if (!vpu_rproc->jtag_enabled)
+		return -EINVAL;
+
+	vpu_write32(vpu_rproc, CORE_DEFAULT2, 0);
+
+	ret = pinctrl_select_state(vpu_rproc->pinctrl,
+				   vpu_rproc->pinctrl_default);
+	if (ret < 0) {
+		dev_err(vpu_rproc->dev,
+			"Failed to configure pins to default\n");
+		return ret;
+	}
+
+	vpu_rproc->jtag_enabled = 0;
+
+	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_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
+	char *buf = vpu_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_vpu_rproc *vpu_rproc = (struct mtk_vpu_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, "1", count) || !strncmp(buf, "enabled", count))
+		ret = vpu_enable_jtag(vpu_rproc);
+	else if (!strncmp(buf, "0", count) || !strncmp(buf, "disabled", count))
+		ret = vpu_disable_jtag(vpu_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 vpu_jtag_probe(struct mtk_vpu_rproc *vpu_rproc)
+{
+	int ret;
+
+	if (!vpu_rproc->rproc->dbg_dir)
+		return -ENODEV;
+
+	vpu_rproc->pinctrl = devm_pinctrl_get(vpu_rproc->dev);
+	if (IS_ERR(vpu_rproc->pinctrl)) {
+		dev_warn(vpu_rproc->dev, "Failed to find JTAG pinctrl\n");
+		return PTR_ERR(vpu_rproc->pinctrl);
+	}
+
+	vpu_rproc->pinctrl_default = pinctrl_lookup_state(vpu_rproc->pinctrl,
+							PINCTRL_STATE_DEFAULT);
+	if (IS_ERR(vpu_rproc->pinctrl_default))
+		return PTR_ERR(vpu_rproc->pinctrl_default);
+
+	vpu_rproc->pinctrl_jtag = pinctrl_lookup_state(vpu_rproc->pinctrl,
+						       "jtag");
+	if (IS_ERR(vpu_rproc->pinctrl_jtag))
+		return PTR_ERR(vpu_rproc->pinctrl_jtag);
+
+	ret = pinctrl_select_state(vpu_rproc->pinctrl,
+				   vpu_rproc->pinctrl_default);
+	if (ret < 0)
+		return ret;
+
+	debugfs_create_file("jtag", 0600, vpu_rproc->rproc->dbg_dir,
+			    vpu_rproc->rproc, &rproc_jtag_ops);
+
+	return 0;
+}
+#endif /* CONFIG_MTK_APU_JTAG */
+
 static int mtk_vpu_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -228,16 +369,16 @@ static int mtk_vpu_rproc_probe(struct platform_device *pdev)
 		goto clk_disable_ipu;
 	}
 
-	vpu_rproc->jtag = devm_clk_get_optional(dev, "jtag");
+	vpu_rproc->jtag = devm_clk_get(vpu_rproc->dev, "jtag");
 	if (IS_ERR(vpu_rproc->jtag)) {
-		dev_err(dev, "Failed to enable jtag clock\n");
+		dev_err(vpu_rproc->dev, "Failed to get jtag clock\n");
 		ret = PTR_ERR(vpu_rproc->jtag);
 		goto clk_disable_axi;
 	}
 
 	ret = clk_prepare_enable(vpu_rproc->jtag);
 	if (ret) {
-		dev_err(dev, "Failed to enable jtag clock\n");
+		dev_err(vpu_rproc->dev, "Failed to enable jtag clock\n");
 		goto clk_disable_axi;
 	}
 
@@ -253,6 +394,12 @@ static int mtk_vpu_rproc_probe(struct platform_device *pdev)
 		goto free_mem;
 	}
 
+#ifdef CONFIG_MTK_APU_JTAG
+	ret = vpu_jtag_probe(vpu_rproc);
+	if (ret)
+		dev_warn(dev, "Failed to configure jtag\n");
+#endif
+
 	return 0;
 
 free_mem:
@@ -277,6 +424,9 @@ static int mtk_vpu_rproc_remove(struct platform_device *pdev)
 
 	disable_irq(vpu_rproc->irq);
 
+#ifdef CONFIG_MTK_APU_JTAG
+	vpu_disable_jtag(vpu_rproc);
+#endif
 	rproc_del(rproc);
 	of_reserved_mem_device_release(dev);
 	clk_disable_unprepare(vpu_rproc->jtag);
-- 
2.26.2

[PATCH 4/6] remoteproc: mtk_vpu_rproc: Don't try to load empty PT_LOAD segment

[Alexandre Bailon]

The firmware generated by our toolchain contains many empty PT_LOAD
segments. The elf loader don't manage it and will raise an error:
"bad phdr da 0x0 mem 0x0".
To workaround it, implement the sanity_check callback to detect the
empty PT_LOAD segment and change it to PT_NULL.
In that way, the elf load won't try to load the segment.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/remoteproc/mtk_apu_rproc.c | 35 +++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/mtk_apu_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
index f2342b747a35..565b3adca5de 100644
--- a/drivers/remoteproc/mtk_apu_rproc.c
+++ b/drivers/remoteproc/mtk_apu_rproc.c
@@ -137,10 +137,39 @@ static void mtk_vpu_rproc_kick(struct rproc *rproc, int vqid)
 	vpu_write32(vpu_rproc, CORE_CTL_XTENSA_INT, 1 << vqid);
 }
 
+int mtk_vpu_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
+{
+	const u8 *elf_data = fw->data;
+	struct elf32_hdr *ehdr;
+	struct elf32_phdr *phdr;
+	int ret;
+	int i;
+
+	ret = rproc_elf_sanity_check(rproc, fw);
+	if (ret)
+		return ret;
+
+	ehdr = (struct elf32_hdr *)elf_data;
+	phdr = (struct elf32_phdr *)(elf_data + ehdr->e_phoff);
+
+	for (i = 0; i < ehdr->e_phnum; i++, phdr++) {
+		/* Remove empty PT_LOAD section */
+		if (phdr->p_type == PT_LOAD && !phdr->p_paddr)
+			phdr->p_type = PT_NULL;
+	}
+
+	return 0;
+}
+
 static const struct rproc_ops mtk_vpu_rproc_ops = {
-	.start		= mtk_vpu_rproc_start,
-	.stop		= mtk_vpu_rproc_stop,
-	.kick		= mtk_vpu_rproc_kick,
+	.start			= mtk_vpu_rproc_start,
+	.stop			= mtk_vpu_rproc_stop,
+	.kick			= mtk_vpu_rproc_kick,
+	.load			= rproc_elf_load_segments,
+	.parse_fw		= rproc_elf_load_rsc_table,
+	.find_loaded_rsc_table	= rproc_elf_find_loaded_rsc_table,
+	.sanity_check		= mtk_vpu_elf_sanity_check,
+	.get_boot_addr		= rproc_elf_get_boot_addr,
 };
 
 static irqreturn_t mtk_vpu_rproc_callback(int irq, void *data)
-- 
2.26.2

[PATCH 5/6] remoteproc: mtk_apu: Don't try to use the APU local RAM

[Alexandre Bailon]

Currently, this local RAM is not accessible from the CPU.
If the CPU tries to access it, then the CPU will hang.

Remoteproc may try to use it when it load a firmware
that has some sections in the local RAM.
This workarounds the issue by skiping this section.

Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
 drivers/remoteproc/mtk_apu_rproc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/remoteproc/mtk_apu_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
index 565b3adca5de..e16d3258a785 100644
--- a/drivers/remoteproc/mtk_apu_rproc.c
+++ b/drivers/remoteproc/mtk_apu_rproc.c
@@ -57,6 +57,9 @@
 #define CORE_DEFAULT2_SPIDEN			BIT(0)
 #define CORE_XTENSA_ALTRESETVEC			(0x000001F8)
 
+#define DRAM0_START				(0x7ff00000)
+#define IRAM0_END				(0x7ff80000)
+
 struct mtk_vpu_rproc {
 	struct device *dev;
 	struct rproc *rproc;
@@ -139,6 +142,7 @@ static void mtk_vpu_rproc_kick(struct rproc *rproc, int vqid)
 
 int mtk_vpu_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
 {
+	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
 	const u8 *elf_data = fw->data;
 	struct elf32_hdr *ehdr;
 	struct elf32_phdr *phdr;
@@ -156,6 +160,16 @@ int mtk_vpu_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
 		/* Remove empty PT_LOAD section */
 		if (phdr->p_type == PT_LOAD && !phdr->p_paddr)
 			phdr->p_type = PT_NULL;
+		/*
+		 * Workaround: Currently, the CPU can't access to the APU
+		 * local RAM. This removes the local RAM section from the
+		 * firmware. Please note that may cause some issues.
+		 */
+		if (phdr->p_paddr >= DRAM0_START && phdr->p_paddr < IRAM0_END) {
+			dev_warn_once(vpu_rproc->dev,
+				      "Skipping the APU local RAM section\n");
+			phdr->p_type = PT_NULL;
+		}
 	}
 
 	return 0;
-- 
2.26.2

[PATCH 6/6] 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>
---
 arch/arm64/boot/dts/mediatek/mt8183.dtsi | 42 ++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 1e03c849dc5d..6a2e9ee0b566 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -720,12 +720,54 @@ ipu_adl: syscon@19010000 {
 			#clock-cells = <1>;
 		};
 
+		vpu0: vpu@0x19100000 {
+			compatible = "mediatek,mt8183-apu";
+			reg = <0 0x19180000 0 0x14000>;
+			reg-names = "mmio";
+			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 = <&vpu_ram>;
+		};
+
 		ipu_core0: syscon@19180000 {
 			compatible = "mediatek,mt8183-ipu_core0", "syscon";
 			reg = <0 0x19180000 0 0x1000>;
 			#clock-cells = <1>;
 		};
 
+		vpu1: vpu@0x19200000 {
+			compatible = "mediatek,mt8183-apu";
+			reg = <0 0x19280000 0 0x14000>;
+			reg-names = "mmio";
+			interrupts = <GIC_SPI 293 IRQ_TYPE_LEVEL_LOW>;
+
+			iommus = <&iommu M4U_PORT_IMG_IPUO>,
+				 <&iommu M4U_PORT_IMG_IPU2O>,
+				 <&iommu M4U_PORT_IMG_IPU3O>,
+				 <&iommu M4U_PORT_IMG_IPUI>,
+				 <&iommu M4U_PORT_IMG_IPU2I>;
+
+			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 = <&scpsys MT8183_POWER_DOMAIN_VPU_CORE1>;
+			memory-region = <&vpu_ram>;
+		};
+
 		ipu_core1: syscon@19280000 {
 			compatible = "mediatek,mt8183-ipu_core1", "syscon";
 			reg = <0 0x19280000 0 0x1000>;
-- 
2.26.2

Re: [PATCH 4/6] remoteproc: mtk_vpu_rproc: Don't try to load empty PT_LOAD segment

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2295 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.8-rc5 next-20200713]
[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  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/20200713-213336
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) 9.3.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
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.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 >>):

>> drivers/remoteproc/mtk_apu_rproc.c:140:5: warning: no previous prototype for 'mtk_vpu_elf_sanity_check' [-Wmissing-prototypes]
     140 | int mtk_vpu_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~

vim +/mtk_vpu_elf_sanity_check +140 drivers/remoteproc/mtk_apu_rproc.c

   139	
 > 140	int mtk_vpu_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
   141	{
   142		const u8 *elf_data = fw->data;
   143		struct elf32_hdr *ehdr;
   144		struct elf32_phdr *phdr;
   145		int ret;
   146		int i;
   147	
   148		ret = rproc_elf_sanity_check(rproc, fw);
   149		if (ret)
   150			return ret;
   151	
   152		ehdr = (struct elf32_hdr *)elf_data;
   153		phdr = (struct elf32_phdr *)(elf_data + ehdr->e_phoff);
   154	
   155		for (i = 0; i < ehdr->e_phnum; i++, phdr++) {
   156			/* Remove empty PT_LOAD section */
   157			if (phdr->p_type == PT_LOAD && !phdr->p_paddr)
   158				phdr->p_type = PT_NULL;
   159		}
   160	
   161		return 0;
   162	}
   163	

---
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: 75712 bytes --]

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

[Rob Herring]

On Mon, 13 Jul 2020 15:29:22 +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          | 121 ++++++++++++++++++
>  1 file changed, 121 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml: properties:pinctrl-names:maxItems: False schema does not allow 2
Documentation/devicetree/bindings/Makefile:20: recipe for target 'Documentation/devicetree/bindings/remoteproc/mtk,apu.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/remoteproc/mtk,apu.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml: ignoring, error in schema: properties: pinctrl-names: maxItems
warning: no schema found in file: ./Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml: ignoring, error in schema: properties: pinctrl-names: maxItems
warning: no schema found in file: ./Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
Makefile:1347: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2


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

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

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

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

[Rob Herring]

On Mon, Jul 13, 2020 at 03:29:22PM +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          | 121 ++++++++++++++++++
>  1 file changed, 121 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 000000000000..1d5fcc135617
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/mtk,apu.yaml
> @@ -0,0 +1,121 @@
> +# 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 Vision Processor Unit (VPU) devices

APU or 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:
> +    description:
> +      Address ranges of the APU MMIO.

Don't put generic descriptions for standard properties.

> +    maxItems: 1
> +
> +  interrupts:
> +    description:
> +      The interrupt number used to receive the interrupts from the DSP.
> +    maxItems: 1
> +
> +  clocks:
> +    description:
> +      "Clocks for co-processor (See: ../clock/clock-bindings.txt)
> +      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

blank line between properties please.

> +  clock-names:
> +    description:
> +      The name of clocks, must be ipu, axi and jtag

That's what the schema says...

> +    items:
> +      - const: ipu
> +      - const: axi
> +      - const: jtag
> +
> +  iommu:
> +    description: "IOMMU (See: ../iommu/iommu.txt)"
> +    maxItems: 3
> +
> +  memory-region:
> +    description: "Reserved memory (See: ../reserved-memory/reserved-memory.txt)"
> +    maxItems: 1
> +
> +  power-domains:
> +    description: "Power domain (see: ../power/power_domain.txt)"
> +    maxItems: 1
> +
> +  pinctrl:
> +    description: pinctrl handles, required to configure pins for JTAG.
> +    maxItems: 2
> +
> +  pinctrl-names:
> +    description:
> +      pinctrl name, must be "default", "jtag".
> +      "default" must configure the pins when JTAG is disabled.
> +      "jtag" must configure the pins for JTAG operations.
> +    items:
> +      - const: default
> +      - const: jtag
> +    maxItems: 2

Don't need maxItems.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - iommu
> +  - memory-region
> +  - power-domains

Add:

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;
> +
> +      vpu_ram: vpu_ram@0x60000000 {
> +        compatible = "shared-dma-pool";
> +        reg = <0 0x60000000 0 0x040000000>;
> +        no-map;
> +        linux,cma-default;
> +      };
> +    };
> +
> +    vpu0: vpu@0x19100000 {

vpu?

Drop the '0x'

> +      compatible = "mediatek,mt8183-apu";
> +      reg = <0 0x19180000 0 0x14000>;

The default cell size for examples is 1.

> +      reg-names = "mmio";
> +      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";

Doesn't match the schema which 'make dt_binding_check' will tell you.

> +
> +      power-domains = <&scpsys MT8183_POWER_DOMAIN_VPU_CORE0>;
> +      memory-region = <&vpu_ram>;
> +    };
> +...
> -- 
> 2.26.2
> 

Re: [PATCH 6/6] ARM64: mt8183: Add support of APU to mt8183

[kernel test robot]

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

Hi Alexandre,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v5.8-rc5 next-20200717]
[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/20200713-213336
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arm64-randconfig-r014-20200717 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ed6b578040a85977026c93bf4188f996148f3218)
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
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

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

All errors (new ones prefixed by >>):

>> Error: arch/arm64/boot/dts/mediatek/mt8183.dtsi:729.21-22 syntax error
   FATAL ERROR: Unable to parse input tree

---
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: 41040 bytes --]

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

[Mathieu Poirier]

On Mon, Jul 13, 2020 at 03:29:23PM +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_rproc.c | 308 +++++++++++++++++++++++++++++

I would name the file mtk_apu.c to be consistent with the existing mtk_scp.c

>  3 files changed, 319 insertions(+)
>  create mode 100644 drivers/remoteproc/mtk_apu_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index c4d1731295eb..e116d4a12ac3 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -42,6 +42,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 e8b886e511f0..2ea231b75fa6 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -12,6 +12,7 @@ remoteproc-y				+= remoteproc_elf_loader.o
>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
>  obj-$(CONFIG_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
> +obj-$(CONFIG_MTK_APU)			+= mtk_apu_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/mtk_apu_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
> new file mode 100644
> index 000000000000..fb416a817ef3
> --- /dev/null
> +++ b/drivers/remoteproc/mtk_apu_rproc.c
> @@ -0,0 +1,308 @@
> +// 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/io.h>
> +#include <linux/iommu.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/highmem.h>

Move this below "delay.h"

> +#include <linux/module.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +
> +#include "remoteproc_internal.h"
> +
> +/* From MT8183 4.5 Vision Processor Unit (VPU).pdf datasheet */
> +#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)

Please don't indent defines.

> +#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)
> +
> +struct mtk_vpu_rproc {
> +	struct device *dev;
> +	struct rproc *rproc;
> +
> +	void __iomem *base;
> +	int irq;
> +	struct clk *axi;
> +	struct clk *ipu;
> +	struct clk *jtag;
> +};
> +
> +static u32 vpu_read32(struct mtk_vpu_rproc *vpu_rproc, u32 off)
> +{
> +	return readl(vpu_rproc->base + off);
> +}
> +
> +static void vpu_write32(struct mtk_vpu_rproc *vpu_rproc, u32 off, u32 value)
> +{
> +	writel(value, vpu_rproc->base + off);
> +}

Not sure that much is gained by adding the above two functions.  Just using
readl/writel would suit me just fine.

> +
> +static int mtk_vpu_rproc_start(struct rproc *rproc)
> +{
> +	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
> +	u32 core_ctrl;
> +
> +	vpu_write32(vpu_rproc, CORE_XTENSA_ALTRESETVEC, rproc->bootaddr);
> +
> +	core_ctrl = vpu_read32(vpu_rproc, 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;
> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
> +
> +	vpu_write32(vpu_rproc, SW_RST, SW_RST_OCD_HALT_ON_RST |
> +				       SW_RST_IPU_B_RST | SW_RST_IPU_D_RST);
> +	ndelay(27);

What is this for?  The state of the VPU can't be polled?

> +	vpu_write32(vpu_rproc, SW_RST, 0);
> +
> +	core_ctrl &= ~CORE_CTRL_PIF_GATED;
> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
> +
> +	vpu_write32(vpu_rproc, CORE_DEFAULT0, CORE_DEFAULT0_AWUSER_USE_IOMMU |
> +					      CORE_DEFAULT0_ARUSER_USE_IOMMU |
> +					      CORE_DEFAULT0_QOS_SWAP_1);
> +	vpu_write32(vpu_rproc, CORE_DEFAULT1,
> +		    CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU |
> +		    CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU);
> +
> +	core_ctrl &= ~CORE_CTRL_RUN_STALL;
> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);

I would certainly appreciate more comments that describe that is going on in
this function.

> +
> +	return 0;
> +}
> +
> +static int mtk_vpu_rproc_stop(struct rproc *rproc)
> +{
> +	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
> +	u32 core_ctrl;
> +
> +	core_ctrl = vpu_read32(vpu_rproc, CORE_CTRL);
> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl | CORE_CTRL_RUN_STALL);
> +
> +	return 0;
> +}
> +
> +static void mtk_vpu_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
> +
> +	vpu_write32(vpu_rproc, CORE_CTL_XTENSA_INT, 1 << vqid);
> +}
> +
> +static const struct rproc_ops mtk_vpu_rproc_ops = {
> +	.start		= mtk_vpu_rproc_start,
> +	.stop		= mtk_vpu_rproc_stop,
> +	.kick		= mtk_vpu_rproc_kick,
> +};
> +
> +static irqreturn_t mtk_vpu_rproc_callback(int irq, void *data)
> +{
> +	struct rproc *rproc = (struct rproc *)data;

There is no need to cast when working with a void pointer.  The same comment
applies throughout.

> +	struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
> +
> +	vpu_write32(vpu_rproc, CORE_XTENSA_INT, 1);
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t handle_event(int irq, void *data)
> +{
> +	struct rproc *rproc = (struct rproc *)data;
> +
> +	rproc_vq_interrupt(rproc, 0);
> +	rproc_vq_interrupt(rproc, 1);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mtk_vpu_rproc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_vpu_rproc *vpu_rproc;
> +	struct rproc *rproc;
> +	struct resource *res;
> +	int ret;
> +
> +	rproc = rproc_alloc(dev, "apu", &mtk_vpu_rproc_ops, NULL,
> +			    sizeof(*vpu_rproc));

The problem with hard coding the name of the remote process is that it work on
only when there is a single processor.  Based on the DTS extention sent with
this serie, there seems to be a possibility of having more the one.  As such
both remote processor will be called "apu", mandating you to look at the
platform resources to know which is which.  Consider using dev_name() or
dev->of_node->name. 

> +	if (!rproc)
> +		return -ENOMEM;
> +
> +	rproc->recovery_disabled = true;
> +	rproc->has_iommu = false;
> +
> +	vpu_rproc = rproc->priv;
> +	vpu_rproc->rproc = rproc;
> +	vpu_rproc->dev = dev;
> +
> +	platform_set_drvdata(pdev, rproc);
> +
> +	rproc->domain = iommu_get_domain_for_dev(dev);
> +	if (!rproc->domain) {
> +		dev_err(dev, "Failed to get the IOMMU domain\n");
> +		ret = -EINVAL;
> +		goto free_rproc;
> +	}
> +
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	vpu_rproc->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(vpu_rproc->base)) {
> +		dev_err(&pdev->dev, "Failed to map mmio\n");

Above dev_err() is used with @dev while here @pdev->dev is.  Please pick one you
like and stick with it. 

> +		ret = PTR_ERR(vpu_rproc->base);
> +		goto free_rproc;
> +	}
> +
> +	vpu_rproc->irq = platform_get_irq(pdev, 0);
> +	if (vpu_rproc->irq < 0) {
> +		ret = vpu_rproc->irq;
> +		goto free_rproc;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, vpu_rproc->irq,
> +					mtk_vpu_rproc_callback, handle_event,
> +					IRQF_SHARED | IRQF_ONESHOT,
> +					"mtk_vpu-remoteproc", rproc);

Same problem as above, i.e hard coding the name of the interrupt will be
confusing when probing sysfs.  Here rproc->index holds the value that
corresponds to 'X' in /sys/dev/class/remoteproc/remoteprocX.  Simply build a
string using that and feed it to devm_request_threaded_ifq().


> +	if (ret) {
> +		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
> +		goto free_rproc;
> +	}
> +
> +	vpu_rproc->ipu = devm_clk_get(dev, "ipu");
> +	if (IS_ERR(vpu_rproc->ipu)) {
> +		dev_err(dev, "Failed to get ipu clock\n");
> +		ret = PTR_ERR(vpu_rproc->ipu);
> +		goto free_rproc;
> +	}
> +
> +	ret = clk_prepare_enable(vpu_rproc->ipu);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable ipu clock\n");
> +		goto free_rproc;
> +	}
> +
> +	vpu_rproc->axi = devm_clk_get(dev, "axi");
> +	if (IS_ERR(vpu_rproc->axi)) {
> +		dev_err(dev, "Failed to get axi clock\n");
> +		ret = PTR_ERR(vpu_rproc->axi);
> +		goto clk_disable_ipu;
> +	}
> +
> +	ret = clk_prepare_enable(vpu_rproc->axi);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable axi clock\n");
> +		goto clk_disable_ipu;
> +	}a

Please look at how Paul use the clock bulk API to deal with multiple clocs in
ingenic_rproc.c and see if it is possible to use the same scheme.

> +
> +	vpu_rproc->jtag = devm_clk_get_optional(dev, "jtag");

Why is the jtag clock optional when the binding document says that it "seems to
be required to run the DSP, even when JTAG is not in use"?

> +	if (IS_ERR(vpu_rproc->jtag)) {
> +		dev_err(dev, "Failed to enable jtag clock\n");
> +		ret = PTR_ERR(vpu_rproc->jtag);
> +		goto clk_disable_axi;
> +	}
> +
> +	ret = clk_prepare_enable(vpu_rproc->jtag);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable jtag clock\n");
> +		goto clk_disable_axi;
> +	}
> +
> +	ret = of_reserved_mem_device_init(dev);
> +	if (ret) {
> +		dev_err(dev, "device does not have specific CMA pool\n");
> +		goto clk_disable_jtag;
> +	}
> +
> +	ret = rproc_add(rproc);
> +	if (ret) {
> +		dev_err(dev, "rproc_add failed: %d\n", ret);
> +		goto free_mem;
> +	}
> +
> +	return 0;
> +
> +free_mem:
> +	of_reserved_mem_device_release(dev);
> +clk_disable_jtag:
> +	clk_disable_unprepare(vpu_rproc->jtag);
> +clk_disable_axi:
> +	clk_disable_unprepare(vpu_rproc->axi);
> +clk_disable_ipu:
> +	clk_disable_unprepare(vpu_rproc->ipu);
> +free_rproc:
> +	rproc_free(rproc);
> +
> +	return ret;
> +}
> +
> +static int mtk_vpu_rproc_remove(struct platform_device *pdev)
> +{
> +	struct rproc *rproc = platform_get_drvdata(pdev);
> +	struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
> +	struct device *dev = &pdev->dev;
> +
> +	disable_irq(vpu_rproc->irq);
> +
> +	rproc_del(rproc);
> +	of_reserved_mem_device_release(dev);
> +	clk_disable_unprepare(vpu_rproc->jtag);
> +	clk_disable_unprepare(vpu_rproc->axi);
> +	clk_disable_unprepare(vpu_rproc->ipu);
> +	rproc_free(rproc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mtk_vpu_rproc_of_match[] __maybe_unused = {

Why is "__maybe_unused" needed?

Thanks,
Mathieu


> +	{ .compatible = "mediatek,mt8183-apu", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_vpu_rproc_of_match);
> +
> +static struct platform_driver mtk_vpu_rproc_driver = {
> +	.probe = mtk_vpu_rproc_probe,
> +	.remove = mtk_vpu_rproc_remove,
> +	.driver = {
> +		.name = "mtk_vpu-rproc",
> +		.of_match_table = of_match_ptr(mtk_vpu_rproc_of_match),
> +	},
> +};
> +module_platform_driver(mtk_vpu_rproc_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Alexandre Bailon");
> +MODULE_DESCRIPTION("Mt8183 VPU Remote Processor control driver");
> -- 
> 2.26.2
> 

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

[Mathieu Poirier]

On Mon, Jul 13, 2020 at 03:29:23PM +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_rproc.c | 308 +++++++++++++++++++++++++++++
>  3 files changed, 319 insertions(+)
>  create mode 100644 drivers/remoteproc/mtk_apu_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index c4d1731295eb..e116d4a12ac3 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -42,6 +42,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 e8b886e511f0..2ea231b75fa6 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -12,6 +12,7 @@ remoteproc-y				+= remoteproc_elf_loader.o
>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
>  obj-$(CONFIG_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
> +obj-$(CONFIG_MTK_APU)			+= mtk_apu_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/mtk_apu_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
> new file mode 100644
> index 000000000000..fb416a817ef3
> --- /dev/null
> +++ b/drivers/remoteproc/mtk_apu_rproc.c
> @@ -0,0 +1,308 @@
> +// 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/io.h>
> +#include <linux/iommu.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/highmem.h>
> +#include <linux/module.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +
> +#include "remoteproc_internal.h"
> +
> +/* From MT8183 4.5 Vision Processor Unit (VPU).pdf datasheet */
> +#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)
> +
> +struct mtk_vpu_rproc {
> +	struct device *dev;
> +	struct rproc *rproc;
> +
> +	void __iomem *base;
> +	int irq;
> +	struct clk *axi;
> +	struct clk *ipu;
> +	struct clk *jtag;
> +};
> +
> +static u32 vpu_read32(struct mtk_vpu_rproc *vpu_rproc, u32 off)
> +{
> +	return readl(vpu_rproc->base + off);
> +}
> +
> +static void vpu_write32(struct mtk_vpu_rproc *vpu_rproc, u32 off, u32 value)
> +{
> +	writel(value, vpu_rproc->base + off);
> +}
> +
> +static int mtk_vpu_rproc_start(struct rproc *rproc)
> +{
> +	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
> +	u32 core_ctrl;
> +
> +	vpu_write32(vpu_rproc, CORE_XTENSA_ALTRESETVEC, rproc->bootaddr);
> +
> +	core_ctrl = vpu_read32(vpu_rproc, 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;
> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
> +
> +	vpu_write32(vpu_rproc, SW_RST, SW_RST_OCD_HALT_ON_RST |
> +				       SW_RST_IPU_B_RST | SW_RST_IPU_D_RST);
> +	ndelay(27);
> +	vpu_write32(vpu_rproc, SW_RST, 0);
> +
> +	core_ctrl &= ~CORE_CTRL_PIF_GATED;
> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
> +
> +	vpu_write32(vpu_rproc, CORE_DEFAULT0, CORE_DEFAULT0_AWUSER_USE_IOMMU |
> +					      CORE_DEFAULT0_ARUSER_USE_IOMMU |
> +					      CORE_DEFAULT0_QOS_SWAP_1);
> +	vpu_write32(vpu_rproc, CORE_DEFAULT1,
> +		    CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU |
> +		    CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU);
> +
> +	core_ctrl &= ~CORE_CTRL_RUN_STALL;
> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
> +
> +	return 0;
> +}
> +
> +static int mtk_vpu_rproc_stop(struct rproc *rproc)
> +{
> +	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
> +	u32 core_ctrl;
> +
> +	core_ctrl = vpu_read32(vpu_rproc, CORE_CTRL);
> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl | CORE_CTRL_RUN_STALL);
> +
> +	return 0;
> +}
> +
> +static void mtk_vpu_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
> +
> +	vpu_write32(vpu_rproc, CORE_CTL_XTENSA_INT, 1 << vqid);
> +}
> +
> +static const struct rproc_ops mtk_vpu_rproc_ops = {
> +	.start		= mtk_vpu_rproc_start,
> +	.stop		= mtk_vpu_rproc_stop,
> +	.kick		= mtk_vpu_rproc_kick,
> +};
> +
> +static irqreturn_t mtk_vpu_rproc_callback(int irq, void *data)
> +{
> +	struct rproc *rproc = (struct rproc *)data;
> +	struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
> +
> +	vpu_write32(vpu_rproc, CORE_XTENSA_INT, 1);
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t handle_event(int irq, void *data)
> +{
> +	struct rproc *rproc = (struct rproc *)data;
> +
> +	rproc_vq_interrupt(rproc, 0);
> +	rproc_vq_interrupt(rproc, 1);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mtk_vpu_rproc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_vpu_rproc *vpu_rproc;
> +	struct rproc *rproc;
> +	struct resource *res;
> +	int ret;
> +
> +	rproc = rproc_alloc(dev, "apu", &mtk_vpu_rproc_ops, NULL,
> +			    sizeof(*vpu_rproc));
> +	if (!rproc)
> +		return -ENOMEM;
> +
> +	rproc->recovery_disabled = true;
> +	rproc->has_iommu = false;
> +
> +	vpu_rproc = rproc->priv;
> +	vpu_rproc->rproc = rproc;
> +	vpu_rproc->dev = dev;
> +
> +	platform_set_drvdata(pdev, rproc);
> +
> +	rproc->domain = iommu_get_domain_for_dev(dev);
> +	if (!rproc->domain) {
> +		dev_err(dev, "Failed to get the IOMMU domain\n");
> +		ret = -EINVAL;
> +		goto free_rproc;
> +	}
> +
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	vpu_rproc->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(vpu_rproc->base)) {
> +		dev_err(&pdev->dev, "Failed to map mmio\n");
> +		ret = PTR_ERR(vpu_rproc->base);
> +		goto free_rproc;
> +	}
> +
> +	vpu_rproc->irq = platform_get_irq(pdev, 0);
> +	if (vpu_rproc->irq < 0) {
> +		ret = vpu_rproc->irq;
> +		goto free_rproc;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, vpu_rproc->irq,
> +					mtk_vpu_rproc_callback, handle_event,
> +					IRQF_SHARED | IRQF_ONESHOT,
> +					"mtk_vpu-remoteproc", rproc);
> +	if (ret) {
> +		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
> +		goto free_rproc;
> +	}
> +
> +	vpu_rproc->ipu = devm_clk_get(dev, "ipu");
> +	if (IS_ERR(vpu_rproc->ipu)) {
> +		dev_err(dev, "Failed to get ipu clock\n");
> +		ret = PTR_ERR(vpu_rproc->ipu);
> +		goto free_rproc;
> +	}
> +
> +	ret = clk_prepare_enable(vpu_rproc->ipu);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable ipu clock\n");
> +		goto free_rproc;
> +	}
> +
> +	vpu_rproc->axi = devm_clk_get(dev, "axi");
> +	if (IS_ERR(vpu_rproc->axi)) {
> +		dev_err(dev, "Failed to get axi clock\n");
> +		ret = PTR_ERR(vpu_rproc->axi);
> +		goto clk_disable_ipu;
> +	}
> +
> +	ret = clk_prepare_enable(vpu_rproc->axi);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable axi clock\n");
> +		goto clk_disable_ipu;
> +	}
> +
> +	vpu_rproc->jtag = devm_clk_get_optional(dev, "jtag");
> +	if (IS_ERR(vpu_rproc->jtag)) {
> +		dev_err(dev, "Failed to enable jtag clock\n");
> +		ret = PTR_ERR(vpu_rproc->jtag);
> +		goto clk_disable_axi;
> +	}
> +
> +	ret = clk_prepare_enable(vpu_rproc->jtag);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable jtag clock\n");
> +		goto clk_disable_axi;
> +	}

I forgot...  Clocks get enabled when the system is booted or the module loaded,
something that is highly inefficient.  Please use rproc->prepare/unprepare() to
deal with clocks at the appropriate time.

> +
> +	ret = of_reserved_mem_device_init(dev);
> +	if (ret) {
> +		dev_err(dev, "device does not have specific CMA pool\n");
> +		goto clk_disable_jtag;
> +	}
> +
> +	ret = rproc_add(rproc);
> +	if (ret) {
> +		dev_err(dev, "rproc_add failed: %d\n", ret);
> +		goto free_mem;
> +	}
> +
> +	return 0;
> +
> +free_mem:
> +	of_reserved_mem_device_release(dev);
> +clk_disable_jtag:
> +	clk_disable_unprepare(vpu_rproc->jtag);
> +clk_disable_axi:
> +	clk_disable_unprepare(vpu_rproc->axi);
> +clk_disable_ipu:
> +	clk_disable_unprepare(vpu_rproc->ipu);
> +free_rproc:
> +	rproc_free(rproc);
> +
> +	return ret;
> +}
> +
> +static int mtk_vpu_rproc_remove(struct platform_device *pdev)
> +{
> +	struct rproc *rproc = platform_get_drvdata(pdev);
> +	struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
> +	struct device *dev = &pdev->dev;
> +
> +	disable_irq(vpu_rproc->irq);
> +
> +	rproc_del(rproc);
> +	of_reserved_mem_device_release(dev);
> +	clk_disable_unprepare(vpu_rproc->jtag);
> +	clk_disable_unprepare(vpu_rproc->axi);
> +	clk_disable_unprepare(vpu_rproc->ipu);
> +	rproc_free(rproc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mtk_vpu_rproc_of_match[] __maybe_unused = {
> +	{ .compatible = "mediatek,mt8183-apu", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_vpu_rproc_of_match);
> +
> +static struct platform_driver mtk_vpu_rproc_driver = {
> +	.probe = mtk_vpu_rproc_probe,
> +	.remove = mtk_vpu_rproc_remove,
> +	.driver = {
> +		.name = "mtk_vpu-rproc",
> +		.of_match_table = of_match_ptr(mtk_vpu_rproc_of_match),
> +	},
> +};
> +module_platform_driver(mtk_vpu_rproc_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Alexandre Bailon");
> +MODULE_DESCRIPTION("Mt8183 VPU Remote Processor control driver");
> -- 
> 2.26.2
> 

Re: [PATCH 3/6] remoteproc: mtk_vpu_rproc: Add support of JTAG

[Mathieu Poirier]

On Mon, Jul 13, 2020 at 03:29:24PM +0200, Alexandre Bailon wrote:
> 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_rproc.c | 156 ++++++++++++++++++++++++++++-
>  2 files changed, 162 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index e116d4a12ac3..e1158563e2e8 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -52,6 +52,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_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
> index fb416a817ef3..f2342b747a35 100644
> --- a/drivers/remoteproc/mtk_apu_rproc.c
> +++ b/drivers/remoteproc/mtk_apu_rproc.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> @@ -14,6 +15,7 @@
>  #include <linux/highmem.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>
>  
> @@ -48,6 +50,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)
>  
>  struct mtk_vpu_rproc {
> @@ -59,6 +66,13 @@ struct mtk_vpu_rproc {
>  	struct clk *axi;
>  	struct clk *ipu;
>  	struct clk *jtag;
> +
> +#ifdef CONFIG_MTK_APU_JTAG
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *pinctrl_default;
> +	struct pinctrl_state *pinctrl_jtag;
> +	bool jtag_enabled;
> +#endif
>  };
>  
>  static u32 vpu_read32(struct mtk_vpu_rproc *vpu_rproc, u32 off)
> @@ -149,6 +163,133 @@ static irqreturn_t handle_event(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +#ifdef CONFIG_MTK_APU_JTAG
> +
> +static int vpu_enable_jtag(struct mtk_vpu_rproc *vpu_rproc)
> +{
> +	int ret = 0;
> +
> +	if (vpu_rproc->jtag_enabled)
> +		return -EINVAL;
> +
> +	ret = pinctrl_select_state(vpu_rproc->pinctrl,
> +				   vpu_rproc->pinctrl_jtag);
> +	if (ret < 0) {
> +		dev_err(vpu_rproc->dev, "Failed to configure pins for JTAG\n");
> +		return ret;
> +	}
> +
> +	vpu_write32(vpu_rproc, CORE_DEFAULT2,
> +		    CORE_DEFAULT2_SPNIDEN | CORE_DEFAULT2_SPIDEN |
> +		    CORE_DEFAULT2_NIDEN | CORE_DEFAULT2_DBG_EN);
> +
> +	vpu_rproc->jtag_enabled = 1;

There should be mutex that gets taken at the beginning and released at the end of
this function.

> +
> +	return ret;
> +}
> +
> +static int vpu_disable_jtag(struct mtk_vpu_rproc *vpu_rproc)
> +{
> +	int ret = 0;
> +
> +	if (!vpu_rproc->jtag_enabled)
> +		return -EINVAL;
> +
> +	vpu_write32(vpu_rproc, CORE_DEFAULT2, 0);
> +
> +	ret = pinctrl_select_state(vpu_rproc->pinctrl,
> +				   vpu_rproc->pinctrl_default);
> +	if (ret < 0) {
> +		dev_err(vpu_rproc->dev,
> +			"Failed to configure pins to default\n");
> +		return ret;
> +	}
> +
> +	vpu_rproc->jtag_enabled = 0;

Same comment as above.

> +
> +	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_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
> +	char *buf = vpu_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_vpu_rproc *vpu_rproc = (struct mtk_vpu_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, "1", count) || !strncmp(buf, "enabled", count))
> +		ret = vpu_enable_jtag(vpu_rproc);
> +	else if (!strncmp(buf, "0", count) || !strncmp(buf, "disabled", count))
> +		ret = vpu_disable_jtag(vpu_rproc);
> +	else
> +		return -EINVAL;

I think we should simply stick with "enabled" and "disabled" to be in line with
what is done in rproc_recovery_write().

> +
> +	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 vpu_jtag_probe(struct mtk_vpu_rproc *vpu_rproc)
> +{
> +	int ret;
> +
> +	if (!vpu_rproc->rproc->dbg_dir)
> +		return -ENODEV;
> +
> +	vpu_rproc->pinctrl = devm_pinctrl_get(vpu_rproc->dev);
> +	if (IS_ERR(vpu_rproc->pinctrl)) {
> +		dev_warn(vpu_rproc->dev, "Failed to find JTAG pinctrl\n");
> +		return PTR_ERR(vpu_rproc->pinctrl);
> +	}
> +
> +	vpu_rproc->pinctrl_default = pinctrl_lookup_state(vpu_rproc->pinctrl,
> +							PINCTRL_STATE_DEFAULT);

Indentation problem.

> +	if (IS_ERR(vpu_rproc->pinctrl_default))
> +		return PTR_ERR(vpu_rproc->pinctrl_default);
> +
> +	vpu_rproc->pinctrl_jtag = pinctrl_lookup_state(vpu_rproc->pinctrl,
> +						       "jtag");
> +	if (IS_ERR(vpu_rproc->pinctrl_jtag))
> +		return PTR_ERR(vpu_rproc->pinctrl_jtag);
> +
> +	ret = pinctrl_select_state(vpu_rproc->pinctrl,
> +				   vpu_rproc->pinctrl_default);

What is the default configuration for?  It does not seem to be needed to
properly boot the remote processor since it is not part of the example in the
bindings or dts patch included in this set.   Moreover it is part of a
configuration option so I really don't understand what it does.



> +	if (ret < 0)
> +		return ret;
> +
> +	debugfs_create_file("jtag", 0600, vpu_rproc->rproc->dbg_dir,
> +			    vpu_rproc->rproc, &rproc_jtag_ops);
> +
> +	return 0;
> +}
> +#endif /* CONFIG_MTK_APU_JTAG */
> +
>  static int mtk_vpu_rproc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -228,16 +369,16 @@ static int mtk_vpu_rproc_probe(struct platform_device *pdev)
>  		goto clk_disable_ipu;
>  	}
>  
> -	vpu_rproc->jtag = devm_clk_get_optional(dev, "jtag");
> +	vpu_rproc->jtag = devm_clk_get(vpu_rproc->dev, "jtag");

As I remarked in my comments on the previous patch, this should have been
devm_clk_get() from the start.  Either that or the bindings are wrong.

>  	if (IS_ERR(vpu_rproc->jtag)) {
> -		dev_err(dev, "Failed to enable jtag clock\n");
> +		dev_err(vpu_rproc->dev, "Failed to get jtag clock\n");

Why go from dev to vpu_rproc->dev?

>  		ret = PTR_ERR(vpu_rproc->jtag);
>  		goto clk_disable_axi;
>  	}
>  
>  	ret = clk_prepare_enable(vpu_rproc->jtag);
>  	if (ret) {
> -		dev_err(dev, "Failed to enable jtag clock\n");
> +		dev_err(vpu_rproc->dev, "Failed to enable jtag clock\n");

Same here.

>  		goto clk_disable_axi;
>  	}
>  
> @@ -253,6 +394,12 @@ static int mtk_vpu_rproc_probe(struct platform_device *pdev)
>  		goto free_mem;
>  	}
>  
> +#ifdef CONFIG_MTK_APU_JTAG
> +	ret = vpu_jtag_probe(vpu_rproc);
> +	if (ret)
> +		dev_warn(dev, "Failed to configure jtag\n");
> +#endif

Please don't use #ifdefs in the code like that.  It is better to introduce a
#else (above) with stubs that don't do anything.  

> +
>  	return 0;
>  
>  free_mem:
> @@ -277,6 +424,9 @@ static int mtk_vpu_rproc_remove(struct platform_device *pdev)
>  
>  	disable_irq(vpu_rproc->irq);
>  
> +#ifdef CONFIG_MTK_APU_JTAG
> +	vpu_disable_jtag(vpu_rproc);
> +#endif
>  	rproc_del(rproc);
>  	of_reserved_mem_device_release(dev);
>  	clk_disable_unprepare(vpu_rproc->jtag);
> -- 
> 2.26.2
> 

Re: [PATCH 4/6] remoteproc: mtk_vpu_rproc: Don't try to load empty PT_LOAD segment

[Mathieu Poirier]

On Mon, Jul 13, 2020 at 03:29:25PM +0200, Alexandre Bailon wrote:
> The firmware generated by our toolchain contains many empty PT_LOAD
> segments. The elf loader don't manage it and will raise an error:
> "bad phdr da 0x0 mem 0x0".
> To workaround it, implement the sanity_check callback to detect the
> empty PT_LOAD segment and change it to PT_NULL.
> In that way, the elf load won't try to load the segment.

This patch doesn't address the real problem, which are empty load segments.  In
my opinion that should be dealt with rather than having to patch things up.  On
the flip side I suspect that you don't control all the process and that systems
are out there with faulty fw images.  As such:

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

> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/remoteproc/mtk_apu_rproc.c | 35 +++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/mtk_apu_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
> index f2342b747a35..565b3adca5de 100644
> --- a/drivers/remoteproc/mtk_apu_rproc.c
> +++ b/drivers/remoteproc/mtk_apu_rproc.c
> @@ -137,10 +137,39 @@ static void mtk_vpu_rproc_kick(struct rproc *rproc, int vqid)
>  	vpu_write32(vpu_rproc, CORE_CTL_XTENSA_INT, 1 << vqid);
>  }
>  
> +int mtk_vpu_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
> +{
> +	const u8 *elf_data = fw->data;
> +	struct elf32_hdr *ehdr;
> +	struct elf32_phdr *phdr;
> +	int ret;
> +	int i;
> +
> +	ret = rproc_elf_sanity_check(rproc, fw);
> +	if (ret)
> +		return ret;
> +
> +	ehdr = (struct elf32_hdr *)elf_data;
> +	phdr = (struct elf32_phdr *)(elf_data + ehdr->e_phoff);
> +
> +	for (i = 0; i < ehdr->e_phnum; i++, phdr++) {
> +		/* Remove empty PT_LOAD section */
> +		if (phdr->p_type == PT_LOAD && !phdr->p_paddr)
> +			phdr->p_type = PT_NULL;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct rproc_ops mtk_vpu_rproc_ops = {
> -	.start		= mtk_vpu_rproc_start,
> -	.stop		= mtk_vpu_rproc_stop,
> -	.kick		= mtk_vpu_rproc_kick,
> +	.start			= mtk_vpu_rproc_start,
> +	.stop			= mtk_vpu_rproc_stop,
> +	.kick			= mtk_vpu_rproc_kick,
> +	.load			= rproc_elf_load_segments,
> +	.parse_fw		= rproc_elf_load_rsc_table,
> +	.find_loaded_rsc_table	= rproc_elf_find_loaded_rsc_table,
> +	.sanity_check		= mtk_vpu_elf_sanity_check,
> +	.get_boot_addr		= rproc_elf_get_boot_addr,
>  };
>  
>  static irqreturn_t mtk_vpu_rproc_callback(int irq, void *data)
> -- 
> 2.26.2
> 

Re: [PATCH 5/6] remoteproc: mtk_apu: Don't try to use the APU local RAM

[Mathieu Poirier]

On Mon, Jul 13, 2020 at 03:29:26PM +0200, Alexandre Bailon wrote:
> Currently, this local RAM is not accessible from the CPU.
> If the CPU tries to access it, then the CPU will hang.
> 
> Remoteproc may try to use it when it load a firmware
> that has some sections in the local RAM.
> This workarounds the issue by skiping this section.
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> ---
>  drivers/remoteproc/mtk_apu_rproc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/remoteproc/mtk_apu_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
> index 565b3adca5de..e16d3258a785 100644
> --- a/drivers/remoteproc/mtk_apu_rproc.c
> +++ b/drivers/remoteproc/mtk_apu_rproc.c
> @@ -57,6 +57,9 @@
>  #define CORE_DEFAULT2_SPIDEN			BIT(0)
>  #define CORE_XTENSA_ALTRESETVEC			(0x000001F8)
>  
> +#define DRAM0_START				(0x7ff00000)
> +#define IRAM0_END				(0x7ff80000)
> +
>  struct mtk_vpu_rproc {
>  	struct device *dev;
>  	struct rproc *rproc;
> @@ -139,6 +142,7 @@ static void mtk_vpu_rproc_kick(struct rproc *rproc, int vqid)
>  
>  int mtk_vpu_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
>  {
> +	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
>  	const u8 *elf_data = fw->data;
>  	struct elf32_hdr *ehdr;
>  	struct elf32_phdr *phdr;
> @@ -156,6 +160,16 @@ int mtk_vpu_elf_sanity_check(struct rproc *rproc, const struct firmware *fw)
>  		/* Remove empty PT_LOAD section */
>  		if (phdr->p_type == PT_LOAD && !phdr->p_paddr)
>  			phdr->p_type = PT_NULL;
> +		/*
> +		 * Workaround: Currently, the CPU can't access to the APU
> +		 * local RAM. This removes the local RAM section from the
> +		 * firmware. Please note that may cause some issues.
> +		 */
> +		if (phdr->p_paddr >= DRAM0_START && phdr->p_paddr < IRAM0_END) {
> +			dev_warn_once(vpu_rproc->dev,
> +				      "Skipping the APU local RAM section\n");
> +			phdr->p_type = PT_NULL;

We can't selectively decide to not load sections of a program due to a platform
driver shortcoming.  Either a real solution is found or booting the remote
processor is interrupted, with a strong incline toward the former.

I guess you're dealing with tightly coupled memory or on-chip RAM areas -
both are accessed on other platform using ioremap_wc() or devm_ioremap_wc().
You might want to try doing what Suman has done in this patchset [1], with
specific attention to TCMs and SRAM.

Thanks,
Mathieu

[1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=310325

> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.26.2
> 

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

[Alexandre Bailon]

Hi Mathieu,

On 7/21/20 12:17 AM, Mathieu Poirier wrote:
> On Mon, Jul 13, 2020 at 03:29:23PM +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_rproc.c | 308 +++++++++++++++++++++++++++++
> I would name the file mtk_apu.c to be consistent with the existing mtk_scp.c
I will rename it
>
>>   3 files changed, 319 insertions(+)
>>   create mode 100644 drivers/remoteproc/mtk_apu_rproc.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index c4d1731295eb..e116d4a12ac3 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -42,6 +42,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 e8b886e511f0..2ea231b75fa6 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -12,6 +12,7 @@ remoteproc-y				+= remoteproc_elf_loader.o
>>   obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
>>   obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
>>   obj-$(CONFIG_MTK_SCP)			+= mtk_scp.o mtk_scp_ipi.o
>> +obj-$(CONFIG_MTK_APU)			+= mtk_apu_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/mtk_apu_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
>> new file mode 100644
>> index 000000000000..fb416a817ef3
>> --- /dev/null
>> +++ b/drivers/remoteproc/mtk_apu_rproc.c
>> @@ -0,0 +1,308 @@
>> +// 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/io.h>
>> +#include <linux/iommu.h>
>> +#include <linux/irq.h>
>> +#include <linux/kernel.h>
>> +#include <linux/highmem.h>
> Move this below "delay.h"
>
>> +#include <linux/module.h>
>> +#include <linux/of_reserved_mem.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc.h>
>> +
>> +#include "remoteproc_internal.h"
>> +
>> +/* From MT8183 4.5 Vision Processor Unit (VPU).pdf datasheet */
>> +#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)
> Please don't indent defines.
>
>> +#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)
>> +
>> +struct mtk_vpu_rproc {
>> +	struct device *dev;
>> +	struct rproc *rproc;
>> +
>> +	void __iomem *base;
>> +	int irq;
>> +	struct clk *axi;
>> +	struct clk *ipu;
>> +	struct clk *jtag;
>> +};
>> +
>> +static u32 vpu_read32(struct mtk_vpu_rproc *vpu_rproc, u32 off)
>> +{
>> +	return readl(vpu_rproc->base + off);
>> +}
>> +
>> +static void vpu_write32(struct mtk_vpu_rproc *vpu_rproc, u32 off, u32 value)
>> +{
>> +	writel(value, vpu_rproc->base + off);
>> +}
> Not sure that much is gained by adding the above two functions.  Just using
> readl/writel would suit me just fine.
I though this was more convenient to use but using readl/writel is also 
fine for me.
>> +
>> +static int mtk_vpu_rproc_start(struct rproc *rproc)
>> +{
>> +	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
>> +	u32 core_ctrl;
>> +
>> +	vpu_write32(vpu_rproc, CORE_XTENSA_ALTRESETVEC, rproc->bootaddr);
>> +
>> +	core_ctrl = vpu_read32(vpu_rproc, 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;
>> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
>> +
>> +	vpu_write32(vpu_rproc, SW_RST, SW_RST_OCD_HALT_ON_RST |
>> +				       SW_RST_IPU_B_RST | SW_RST_IPU_D_RST);
>> +	ndelay(27);
> What is this for?  The state of the VPU can't be polled?

TBH, I don't know. I got the programming model from Mediatek's kernel.

I assumed that was the minimum time required to maintain reset asserted 
to make it effective.

>
>> +	vpu_write32(vpu_rproc, SW_RST, 0);
>> +
>> +	core_ctrl &= ~CORE_CTRL_PIF_GATED;
>> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
>> +
>> +	vpu_write32(vpu_rproc, CORE_DEFAULT0, CORE_DEFAULT0_AWUSER_USE_IOMMU |
>> +					      CORE_DEFAULT0_ARUSER_USE_IOMMU |
>> +					      CORE_DEFAULT0_QOS_SWAP_1);
>> +	vpu_write32(vpu_rproc, CORE_DEFAULT1,
>> +		    CORE_DEFAULT0_AWUSER_IDMA_USE_IOMMU |
>> +		    CORE_DEFAULT0_ARUSER_IDMA_USE_IOMMU);
>> +
>> +	core_ctrl &= ~CORE_CTRL_RUN_STALL;
>> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl);
> I would certainly appreciate more comments that describe that is going on in
> this function.
I will try to comment a little more but again, this come from Mediatek's 
kernel.
Even if I have access to the datasheet, there are no much details.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int mtk_vpu_rproc_stop(struct rproc *rproc)
>> +{
>> +	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
>> +	u32 core_ctrl;
>> +
>> +	core_ctrl = vpu_read32(vpu_rproc, CORE_CTRL);
>> +	vpu_write32(vpu_rproc, CORE_CTRL, core_ctrl | CORE_CTRL_RUN_STALL);
>> +
>> +	return 0;
>> +}
>> +
>> +static void mtk_vpu_rproc_kick(struct rproc *rproc, int vqid)
>> +{
>> +	struct mtk_vpu_rproc *vpu_rproc = rproc->priv;
>> +
>> +	vpu_write32(vpu_rproc, CORE_CTL_XTENSA_INT, 1 << vqid);
>> +}
>> +
>> +static const struct rproc_ops mtk_vpu_rproc_ops = {
>> +	.start		= mtk_vpu_rproc_start,
>> +	.stop		= mtk_vpu_rproc_stop,
>> +	.kick		= mtk_vpu_rproc_kick,
>> +};
>> +
>> +static irqreturn_t mtk_vpu_rproc_callback(int irq, void *data)
>> +{
>> +	struct rproc *rproc = (struct rproc *)data;
> There is no need to cast when working with a void pointer.  The same comment
> applies throughout.
>
>> +	struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
>> +
>> +	vpu_write32(vpu_rproc, CORE_XTENSA_INT, 1);
>> +
>> +	return IRQ_WAKE_THREAD;
>> +}
>> +
>> +static irqreturn_t handle_event(int irq, void *data)
>> +{
>> +	struct rproc *rproc = (struct rproc *)data;
>> +
>> +	rproc_vq_interrupt(rproc, 0);
>> +	rproc_vq_interrupt(rproc, 1);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int mtk_vpu_rproc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct mtk_vpu_rproc *vpu_rproc;
>> +	struct rproc *rproc;
>> +	struct resource *res;
>> +	int ret;
>> +
>> +	rproc = rproc_alloc(dev, "apu", &mtk_vpu_rproc_ops, NULL,
>> +			    sizeof(*vpu_rproc));
> The problem with hard coding the name of the remote process is that it work on
> only when there is a single processor.  Based on the DTS extention sent with
> this serie, there seems to be a possibility of having more the one.  As such
> both remote processor will be called "apu", mandating you to look at the
> platform resources to know which is which.  Consider using dev_name() or
> dev->of_node->name.
>
>> +	if (!rproc)
>> +		return -ENOMEM;
>> +
>> +	rproc->recovery_disabled = true;
>> +	rproc->has_iommu = false;
>> +
>> +	vpu_rproc = rproc->priv;
>> +	vpu_rproc->rproc = rproc;
>> +	vpu_rproc->dev = dev;
>> +
>> +	platform_set_drvdata(pdev, rproc);
>> +
>> +	rproc->domain = iommu_get_domain_for_dev(dev);
>> +	if (!rproc->domain) {
>> +		dev_err(dev, "Failed to get the IOMMU domain\n");
>> +		ret = -EINVAL;
>> +		goto free_rproc;
>> +	}
>> +
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	vpu_rproc->base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(vpu_rproc->base)) {
>> +		dev_err(&pdev->dev, "Failed to map mmio\n");
> Above dev_err() is used with @dev while here @pdev->dev is.  Please pick one you
> like and stick with it.
>
>> +		ret = PTR_ERR(vpu_rproc->base);
>> +		goto free_rproc;
>> +	}
>> +
>> +	vpu_rproc->irq = platform_get_irq(pdev, 0);
>> +	if (vpu_rproc->irq < 0) {
>> +		ret = vpu_rproc->irq;
>> +		goto free_rproc;
>> +	}
>> +
>> +	ret = devm_request_threaded_irq(dev, vpu_rproc->irq,
>> +					mtk_vpu_rproc_callback, handle_event,
>> +					IRQF_SHARED | IRQF_ONESHOT,
>> +					"mtk_vpu-remoteproc", rproc);
> Same problem as above, i.e hard coding the name of the interrupt will be
> confusing when probing sysfs.  Here rproc->index holds the value that
> corresponds to 'X' in /sys/dev/class/remoteproc/remoteprocX.  Simply build a
> string using that and feed it to devm_request_threaded_ifq().
>
>
>> +	if (ret) {
>> +		dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
>> +		goto free_rproc;
>> +	}
>> +
>> +	vpu_rproc->ipu = devm_clk_get(dev, "ipu");
>> +	if (IS_ERR(vpu_rproc->ipu)) {
>> +		dev_err(dev, "Failed to get ipu clock\n");
>> +		ret = PTR_ERR(vpu_rproc->ipu);
>> +		goto free_rproc;
>> +	}
>> +
>> +	ret = clk_prepare_enable(vpu_rproc->ipu);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable ipu clock\n");
>> +		goto free_rproc;
>> +	}
>> +
>> +	vpu_rproc->axi = devm_clk_get(dev, "axi");
>> +	if (IS_ERR(vpu_rproc->axi)) {
>> +		dev_err(dev, "Failed to get axi clock\n");
>> +		ret = PTR_ERR(vpu_rproc->axi);
>> +		goto clk_disable_ipu;
>> +	}
>> +
>> +	ret = clk_prepare_enable(vpu_rproc->axi);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable axi clock\n");
>> +		goto clk_disable_ipu;
>> +	}a
> Please look at how Paul use the clock bulk API to deal with multiple clocs in
> ingenic_rproc.c and see if it is possible to use the same scheme.
I did not knew the bulk API. I think it should work.
>
>> +
>> +	vpu_rproc->jtag = devm_clk_get_optional(dev, "jtag");
> Why is the jtag clock optional when the binding document says that it "seems to
> be required to run the DSP, even when JTAG is not in use"?

I forget to change it to devm_clk_get when I figured out this was 
actually not optional.

Thanks,
Alexandre

>
>> +	if (IS_ERR(vpu_rproc->jtag)) {
>> +		dev_err(dev, "Failed to enable jtag clock\n");
>> +		ret = PTR_ERR(vpu_rproc->jtag);
>> +		goto clk_disable_axi;
>> +	}
>> +
>> +	ret = clk_prepare_enable(vpu_rproc->jtag);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to enable jtag clock\n");
>> +		goto clk_disable_axi;
>> +	}
>> +
>> +	ret = of_reserved_mem_device_init(dev);
>> +	if (ret) {
>> +		dev_err(dev, "device does not have specific CMA pool\n");
>> +		goto clk_disable_jtag;
>> +	}
>> +
>> +	ret = rproc_add(rproc);
>> +	if (ret) {
>> +		dev_err(dev, "rproc_add failed: %d\n", ret);
>> +		goto free_mem;
>> +	}
>> +
>> +	return 0;
>> +
>> +free_mem:
>> +	of_reserved_mem_device_release(dev);
>> +clk_disable_jtag:
>> +	clk_disable_unprepare(vpu_rproc->jtag);
>> +clk_disable_axi:
>> +	clk_disable_unprepare(vpu_rproc->axi);
>> +clk_disable_ipu:
>> +	clk_disable_unprepare(vpu_rproc->ipu);
>> +free_rproc:
>> +	rproc_free(rproc);
>> +
>> +	return ret;
>> +}
>> +
>> +static int mtk_vpu_rproc_remove(struct platform_device *pdev)
>> +{
>> +	struct rproc *rproc = platform_get_drvdata(pdev);
>> +	struct mtk_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	disable_irq(vpu_rproc->irq);
>> +
>> +	rproc_del(rproc);
>> +	of_reserved_mem_device_release(dev);
>> +	clk_disable_unprepare(vpu_rproc->jtag);
>> +	clk_disable_unprepare(vpu_rproc->axi);
>> +	clk_disable_unprepare(vpu_rproc->ipu);
>> +	rproc_free(rproc);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id mtk_vpu_rproc_of_match[] __maybe_unused = {
> Why is "__maybe_unused" needed?
>
> Thanks,
> Mathieu
>
>
>> +	{ .compatible = "mediatek,mt8183-apu", },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, mtk_vpu_rproc_of_match);
>> +
>> +static struct platform_driver mtk_vpu_rproc_driver = {
>> +	.probe = mtk_vpu_rproc_probe,
>> +	.remove = mtk_vpu_rproc_remove,
>> +	.driver = {
>> +		.name = "mtk_vpu-rproc",
>> +		.of_match_table = of_match_ptr(mtk_vpu_rproc_of_match),
>> +	},
>> +};
>> +module_platform_driver(mtk_vpu_rproc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Alexandre Bailon");
>> +MODULE_DESCRIPTION("Mt8183 VPU Remote Processor control driver");
>> -- 
>> 2.26.2
>>

Re: [PATCH 3/6] remoteproc: mtk_vpu_rproc: Add support of JTAG

[Alexandre Bailon]

On 7/21/20 9:52 PM, Mathieu Poirier wrote:
> On Mon, Jul 13, 2020 at 03:29:24PM +0200, Alexandre Bailon wrote:
>> 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_rproc.c | 156 ++++++++++++++++++++++++++++-
>>   2 files changed, 162 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index e116d4a12ac3..e1158563e2e8 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -52,6 +52,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_rproc.c b/drivers/remoteproc/mtk_apu_rproc.c
>> index fb416a817ef3..f2342b747a35 100644
>> --- a/drivers/remoteproc/mtk_apu_rproc.c
>> +++ b/drivers/remoteproc/mtk_apu_rproc.c
>> @@ -5,6 +5,7 @@
>>   
>>   #include <linux/bitops.h>
>>   #include <linux/clk.h>
>> +#include <linux/debugfs.h>
>>   #include <linux/delay.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/io.h>
>> @@ -14,6 +15,7 @@
>>   #include <linux/highmem.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>
>>   
>> @@ -48,6 +50,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)
>>   
>>   struct mtk_vpu_rproc {
>> @@ -59,6 +66,13 @@ struct mtk_vpu_rproc {
>>   	struct clk *axi;
>>   	struct clk *ipu;
>>   	struct clk *jtag;
>> +
>> +#ifdef CONFIG_MTK_APU_JTAG
>> +	struct pinctrl *pinctrl;
>> +	struct pinctrl_state *pinctrl_default;
>> +	struct pinctrl_state *pinctrl_jtag;
>> +	bool jtag_enabled;
>> +#endif
>>   };
>>   
>>   static u32 vpu_read32(struct mtk_vpu_rproc *vpu_rproc, u32 off)
>> @@ -149,6 +163,133 @@ static irqreturn_t handle_event(int irq, void *data)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> +#ifdef CONFIG_MTK_APU_JTAG
>> +
>> +static int vpu_enable_jtag(struct mtk_vpu_rproc *vpu_rproc)
>> +{
>> +	int ret = 0;
>> +
>> +	if (vpu_rproc->jtag_enabled)
>> +		return -EINVAL;
>> +
>> +	ret = pinctrl_select_state(vpu_rproc->pinctrl,
>> +				   vpu_rproc->pinctrl_jtag);
>> +	if (ret < 0) {
>> +		dev_err(vpu_rproc->dev, "Failed to configure pins for JTAG\n");
>> +		return ret;
>> +	}
>> +
>> +	vpu_write32(vpu_rproc, CORE_DEFAULT2,
>> +		    CORE_DEFAULT2_SPNIDEN | CORE_DEFAULT2_SPIDEN |
>> +		    CORE_DEFAULT2_NIDEN | CORE_DEFAULT2_DBG_EN);
>> +
>> +	vpu_rproc->jtag_enabled = 1;
> There should be mutex that gets taken at the beginning and released at the end of
> this function.
>
>> +
>> +	return ret;
>> +}
>> +
>> +static int vpu_disable_jtag(struct mtk_vpu_rproc *vpu_rproc)
>> +{
>> +	int ret = 0;
>> +
>> +	if (!vpu_rproc->jtag_enabled)
>> +		return -EINVAL;
>> +
>> +	vpu_write32(vpu_rproc, CORE_DEFAULT2, 0);
>> +
>> +	ret = pinctrl_select_state(vpu_rproc->pinctrl,
>> +				   vpu_rproc->pinctrl_default);
>> +	if (ret < 0) {
>> +		dev_err(vpu_rproc->dev,
>> +			"Failed to configure pins to default\n");
>> +		return ret;
>> +	}
>> +
>> +	vpu_rproc->jtag_enabled = 0;
> Same comment as above.
>
>> +
>> +	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_vpu_rproc *vpu_rproc = (struct mtk_vpu_rproc *)rproc->priv;
>> +	char *buf = vpu_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_vpu_rproc *vpu_rproc = (struct mtk_vpu_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, "1", count) || !strncmp(buf, "enabled", count))
>> +		ret = vpu_enable_jtag(vpu_rproc);
>> +	else if (!strncmp(buf, "0", count) || !strncmp(buf, "disabled", count))
>> +		ret = vpu_disable_jtag(vpu_rproc);
>> +	else
>> +		return -EINVAL;
> I think we should simply stick with "enabled" and "disabled" to be in line with
> what is done in rproc_recovery_write().
>
>> +
>> +	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 vpu_jtag_probe(struct mtk_vpu_rproc *vpu_rproc)
>> +{
>> +	int ret;
>> +
>> +	if (!vpu_rproc->rproc->dbg_dir)
>> +		return -ENODEV;
>> +
>> +	vpu_rproc->pinctrl = devm_pinctrl_get(vpu_rproc->dev);
>> +	if (IS_ERR(vpu_rproc->pinctrl)) {
>> +		dev_warn(vpu_rproc->dev, "Failed to find JTAG pinctrl\n");
>> +		return PTR_ERR(vpu_rproc->pinctrl);
>> +	}
>> +
>> +	vpu_rproc->pinctrl_default = pinctrl_lookup_state(vpu_rproc->pinctrl,
>> +							PINCTRL_STATE_DEFAULT);
> Indentation problem.
>
>> +	if (IS_ERR(vpu_rproc->pinctrl_default))
>> +		return PTR_ERR(vpu_rproc->pinctrl_default);
>> +
>> +	vpu_rproc->pinctrl_jtag = pinctrl_lookup_state(vpu_rproc->pinctrl,
>> +						       "jtag");
>> +	if (IS_ERR(vpu_rproc->pinctrl_jtag))
>> +		return PTR_ERR(vpu_rproc->pinctrl_jtag);
>> +
>> +	ret = pinctrl_select_state(vpu_rproc->pinctrl,
>> +				   vpu_rproc->pinctrl_default);
> What is the default configuration for?  It does not seem to be needed to
> properly boot the remote processor since it is not part of the example in the
> bindings or dts patch included in this set.   Moreover it is part of a
> configuration option so I really don't understand what it does.

I have a poor knowledge of pinctrl framework so I may have done things 
wrong here.
This is not really needed for the remote processor.
By default, I don't want pin to be configured for JTAG until we enable 
it and
I want to be able to revert it the default state.
May be this is too much and I should assume that if we build the driver 
with JTAG enabled
then we want the pins to be configured for JTAG by default.

>
>
>
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	debugfs_create_file("jtag", 0600, vpu_rproc->rproc->dbg_dir,
>> +			    vpu_rproc->rproc, &rproc_jtag_ops);
>> +
>> +	return 0;
>> +}
>> +#endif /* CONFIG_MTK_APU_JTAG */
>> +
>>   static int mtk_vpu_rproc_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>> @@ -228,16 +369,16 @@ static int mtk_vpu_rproc_probe(struct platform_device *pdev)
>>   		goto clk_disable_ipu;
>>   	}
>>   
>> -	vpu_rproc->jtag = devm_clk_get_optional(dev, "jtag");
>> +	vpu_rproc->jtag = devm_clk_get(vpu_rproc->dev, "jtag");
> As I remarked in my comments on the previous patch, this should have been
> devm_clk_get() from the start.  Either that or the bindings are wrong.

I should have not made the change in this patch. I will fix it.

Thanks,
Alexandre

>
>>   	if (IS_ERR(vpu_rproc->jtag)) {
>> -		dev_err(dev, "Failed to enable jtag clock\n");
>> +		dev_err(vpu_rproc->dev, "Failed to get jtag clock\n");
> Why go from dev to vpu_rproc->dev?
>
>>   		ret = PTR_ERR(vpu_rproc->jtag);
>>   		goto clk_disable_axi;
>>   	}
>>   
>>   	ret = clk_prepare_enable(vpu_rproc->jtag);
>>   	if (ret) {
>> -		dev_err(dev, "Failed to enable jtag clock\n");
>> +		dev_err(vpu_rproc->dev, "Failed to enable jtag clock\n");
> Same here.
>
>>   		goto clk_disable_axi;
>>   	}
>>   
>> @@ -253,6 +394,12 @@ static int mtk_vpu_rproc_probe(struct platform_device *pdev)
>>   		goto free_mem;
>>   	}
>>   
>> +#ifdef CONFIG_MTK_APU_JTAG
>> +	ret = vpu_jtag_probe(vpu_rproc);
>> +	if (ret)
>> +		dev_warn(dev, "Failed to configure jtag\n");
>> +#endif
> Please don't use #ifdefs in the code like that.  It is better to introduce a
> #else (above) with stubs that don't do anything.
>
>> +
>>   	return 0;
>>   
>>   free_mem:
>> @@ -277,6 +424,9 @@ static int mtk_vpu_rproc_remove(struct platform_device *pdev)
>>   
>>   	disable_irq(vpu_rproc->irq);
>>   
>> +#ifdef CONFIG_MTK_APU_JTAG
>> +	vpu_disable_jtag(vpu_rproc);
>> +#endif
>>   	rproc_del(rproc);
>>   	of_reserved_mem_device_release(dev);
>>   	clk_disable_unprepare(vpu_rproc->jtag);
>> -- 
>> 2.26.2
>>