[PATCH v2 0/9] Add support for MT8195 SCP 2nd core

[PATCH v2 0/9] Add support for MT8195 SCP 2nd core

[Tinghan Shen]

The mediatek remoteproc driver currently only allows bringing up a 
single core SCP, e.g. MT8183. It also only bringing up the 1st 
core in SoCs with a dual-core SCP, e.g. MT8195. This series support 
to bring-up the 2nd core of the dual-core SCP.

Tinghan Shen (9):
  dt-binding: remoteproc: mediatek: Support dual-core SCP
  remoteproc: mediatek: Support hanlding scp core 1 wdt timeout
  remoteproc: mediatek: Add SCP core 1 register definitions
  remoteproc: mediatek: Support probing for the 2nd core of dual-core
    SCP
  remoteproc: mediatek: Add chip dependent operations for SCP core 1
  remoteproc: mediatek: Add SCP core 1 SRAM offset
  remoteproc: mediatek: Add SCP core 1 as a rproc subdevice
  remoteproc: mediatek: Wait SCP core 1 probe done
  mfd: cros_ec: Add SCP core 1 as a new CrOS EC MCU

 .../bindings/remoteproc/mtk,scp.yaml          |  13 +
 drivers/mfd/cros_ec_dev.c                     |   5 +
 drivers/remoteproc/mtk_common.h               |  35 ++
 drivers/remoteproc/mtk_scp.c                  | 322 +++++++++++++++++-
 .../linux/platform_data/cros_ec_commands.h    |   2 +
 include/linux/platform_data/cros_ec_proto.h   |   1 +
 6 files changed, 375 insertions(+), 3 deletions(-)

-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/9] dt-binding: remoteproc: mediatek: Support dual-core SCP

[Tinghan Shen]

The MT8195 SCP co-processor is a dual-core RISC-V MCU.

Add a new property to reference the sibling core and to assign
core id to SCP nodes. Also add a new compatile for the driver of
SCP 2nd core.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
---
 .../devicetree/bindings/remoteproc/mtk,scp.yaml     | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
index eec3b9c4c713..4576ff9b1f2d 100644
--- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
@@ -20,6 +20,7 @@ properties:
       - mediatek,mt8186-scp
       - mediatek,mt8192-scp
       - mediatek,mt8195-scp
+      - mediatek,mt8195-scp-dual
 
   reg:
     description:
@@ -57,6 +58,17 @@ properties:
   memory-region:
     maxItems: 1
 
+  mediatek,scp-core:
+    $ref: "/schemas/types.yaml#/definitions/phandle-array"
+    description:
+      Reference to the sibling SCP core. This is required to
+      enable support for dual-core SCP.
+    items:
+      - items:
+          - description: Phandle of sibling SCP node.
+          - description: Core id of this SCP node
+            enum: [0, 1]
+
 required:
   - compatible
   - reg
@@ -115,6 +127,7 @@ examples:
         reg-names = "sram", "cfg", "l1tcm";
         clocks = <&infracfg CLK_INFRA_SCPSYS>;
         clock-names = "main";
+        mediatek,scp-core = <&scp_dual 0>;
 
         cros_ec {
             mediatek,rpmsg-name = "cros-ec-rpmsg";
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/9] remoteproc: mediatek: Support hanlding scp core 1 wdt timeout

[Tinghan Shen]

MT8195 SCP is a dual-core processor. The SCP core 1 watchdog timeout
interrupt uses the same interrupt line of SCP core 0 watchdog timeout
interrupt.

Add support for handling SCP core 1 watchdog timeout interrupt in the
SCP IRQ handler.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
---
 drivers/remoteproc/mtk_common.h |  4 ++++
 drivers/remoteproc/mtk_scp.c    | 27 ++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
index ea6fa1100a00..73e8adf00de3 100644
--- a/drivers/remoteproc/mtk_common.h
+++ b/drivers/remoteproc/mtk_common.h
@@ -54,6 +54,10 @@
 #define MT8192_CORE0_WDT_IRQ		0x10030
 #define MT8192_CORE0_WDT_CFG		0x10034
 
+#define MT8195_SYS_STATUS		0x4004
+#define MT8195_CORE0_WDT		BIT(16)
+#define MT8195_CORE1_WDT		BIT(17)
+
 #define MT8195_L1TCM_SRAM_PDN_RESERVED_RSI_BITS		GENMASK(7, 4)
 
 #define SCP_FW_VER_LEN			32
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index 47b2a40e1b4a..3510c6d0bbc8 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -212,6 +212,31 @@ static void mt8192_scp_irq_handler(struct mtk_scp *scp)
 	}
 }
 
+static void mt8195_scp_irq_handler(struct mtk_scp *scp)
+{
+	u32 scp_to_host;
+
+	scp_to_host = readl(scp->reg_base + MT8192_SCP2APMCU_IPC_SET);
+
+	if (scp_to_host & MT8192_SCP_IPC_INT_BIT) {
+		scp_ipi_handler(scp);
+
+		/*
+		 * SCP won't send another interrupt until we clear
+		 * MT8192_SCP2APMCU_IPC.
+		 */
+		writel(MT8192_SCP_IPC_INT_BIT,
+		       scp->reg_base + MT8192_SCP2APMCU_IPC_CLR);
+	} else {
+		if (readl(scp->reg_base + MT8195_SYS_STATUS) & MT8195_CORE1_WDT) {
+			writel(1, scp->reg_base + MT8195_CORE1_WDT_IRQ);
+		} else {
+			writel(1, scp->reg_base + MT8192_CORE0_WDT_IRQ);
+			scp_wdt_handler(scp, scp_to_host);
+		}
+	}
+}
+
 static irqreturn_t scp_irq_handler(int irq, void *priv)
 {
 	struct mtk_scp *scp = priv;
@@ -961,7 +986,7 @@ static const struct mtk_scp_of_data mt8192_of_data = {
 static const struct mtk_scp_of_data mt8195_of_data = {
 	.scp_clk_get = mt8195_scp_clk_get,
 	.scp_before_load = mt8195_scp_before_load,
-	.scp_irq_handler = mt8192_scp_irq_handler,
+	.scp_irq_handler = mt8195_scp_irq_handler,
 	.scp_reset_assert = mt8192_scp_reset_assert,
 	.scp_reset_deassert = mt8192_scp_reset_deassert,
 	.scp_stop = mt8195_scp_stop,
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/9] remoteproc: mediatek: Add SCP core 1 register definitions

[Tinghan Shen]

Add MT8195 SCP core 1 related register definitions.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
---
 drivers/remoteproc/mtk_common.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
index 73e8adf00de3..5582f4207fbf 100644
--- a/drivers/remoteproc/mtk_common.h
+++ b/drivers/remoteproc/mtk_common.h
@@ -47,6 +47,7 @@
 #define MT8192_SCP2SPM_IPC_CLR		0x4094
 #define MT8192_GIPC_IN_SET		0x4098
 #define MT8192_HOST_IPC_INT_BIT		BIT(0)
+#define MT8195_CORE1_HOST_IPC_INT_BIT	BIT(4)
 
 #define MT8192_CORE0_SW_RSTN_CLR	0x10000
 #define MT8192_CORE0_SW_RSTN_SET	0x10004
@@ -60,6 +61,26 @@
 
 #define MT8195_L1TCM_SRAM_PDN_RESERVED_RSI_BITS		GENMASK(7, 4)
 
+#define MT8195_CPU1_SRAM_PD			0x1084
+#define MT8195_SSHUB2APMCU_IPC_SET		0x4088
+#define MT8195_SSHUB2APMCU_IPC_CLR		0x408C
+#define MT8195_CORE1_SW_RSTN_CLR		0x20000
+#define MT8195_CORE1_SW_RSTN_SET		0x20004
+#define MT8195_CORE1_MEM_ATT_PREDEF		0x20008
+#define MT8195_CORE1_WDT_IRQ			0x20030
+#define MT8195_CORE1_WDT_CFG			0x20034
+
+#define MT8195_SEC_CTRL				0x85000
+#define MT8195_CORE_OFFSET_ENABLE_D		BIT(13)
+#define MT8195_CORE_OFFSET_ENABLE_I		BIT(12)
+#define MT8195_L2TCM_OFFSET_RANGE_0_LOW		0x850b0
+#define MT8195_L2TCM_OFFSET_RANGE_0_HIGH	0x850b4
+#define MT8195_L2TCM_OFFSET			0x850d0
+#define SCP_SRAM_REMAP_LOW			0
+#define SCP_SRAM_REMAP_HIGH			1
+#define SCP_SRAM_REMAP_OFFSET			2
+#define SCP_SRAM_REMAP_SIZE			3
+
 #define SCP_FW_VER_LEN			32
 #define SCP_SHARE_BUFFER_SIZE		288
 
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP

[Tinghan Shen]

The mtk_scp.c driver only supports the single core SCP and the
1st core of a dual-core SCP. This patch extends it for the 2nd core.

MT8195 SCP is a dual-core MCU. Both cores are housed in the same subsys.
They have the same viewpoint of registers and memory.

Core 1 of the SCP features its own set of core configuration registers,
interrupt controller, timers, and DMAs. The rest of the peripherals
in this subsystem are shared by core 0 and core 1.

As for memory, core 1 has its own cache memory. the SCP SRAM is shared
by core 0 and core 1.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
---
 drivers/remoteproc/mtk_scp.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index 3510c6d0bbc8..91b4aefde4ac 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -23,6 +23,10 @@
 #define MAX_CODE_SIZE 0x500000
 #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
 
+#define SCP_CORE_0 0
+#define SCP_CORE_1 1
+#define SCP_CORE_SINGLE 0xF
+
 /**
  * scp_get() - get a reference to SCP.
  *
@@ -836,6 +840,7 @@ static int scp_probe(struct platform_device *pdev)
 	struct resource *res;
 	const char *fw_name = "scp.img";
 	int ret, i;
+	u32 core_id = SCP_CORE_SINGLE;
 
 	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
 	if (ret < 0 && ret != -EINVAL)
@@ -851,8 +856,16 @@ static int scp_probe(struct platform_device *pdev)
 	scp->data = of_device_get_match_data(dev);
 	platform_set_drvdata(pdev, scp);
 
+	ret = of_property_read_u32_index(dev->of_node, "mediatek,scp-core", 1, &core_id);
+	if (ret == 0)
+		dev_info(dev, "Boot SCP dual core %u\n", core_id);
+
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
-	scp->sram_base = devm_ioremap_resource(dev, res);
+	if (core_id == SCP_CORE_1)
+		scp->sram_base = devm_ioremap(dev, res->start, resource_size(res));
+	else
+		scp->sram_base = devm_ioremap_resource(dev, res);
+
 	if (IS_ERR(scp->sram_base))
 		return dev_err_probe(dev, PTR_ERR(scp->sram_base),
 				     "Failed to parse and map sram memory\n");
@@ -873,7 +886,12 @@ static int scp_probe(struct platform_device *pdev)
 		scp->l1tcm_phys = res->start;
 	}
 
-	scp->reg_base = devm_platform_ioremap_resource_byname(pdev, "cfg");
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
+	if (core_id == SCP_CORE_1)
+		scp->reg_base = devm_ioremap(dev, res->start, resource_size(res));
+	else
+		scp->reg_base = devm_ioremap_resource(dev, res);
+
 	if (IS_ERR(scp->reg_base))
 		return dev_err_probe(dev, PTR_ERR(scp->reg_base),
 				     "Failed to parse and map cfg memory\n");
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/9] remoteproc: mediatek: Add chip dependent operations for SCP core 1

[Tinghan Shen]

The SCP rproc operations has chip dependent callbacks. Implement a
version of these callbacks for MT8195 SCP core 1.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
---
 drivers/remoteproc/mtk_scp.c | 65 ++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index 91b4aefde4ac..731a8094c373 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -180,6 +180,16 @@ static void mt8192_scp_reset_deassert(struct mtk_scp *scp)
 	writel(1, scp->reg_base + MT8192_CORE0_SW_RSTN_CLR);
 }
 
+static void mt8195_scp_dual_reset_assert(struct mtk_scp *scp)
+{
+	writel(1, scp->reg_base + MT8195_CORE1_SW_RSTN_SET);
+}
+
+static void mt8195_scp_dual_reset_deassert(struct mtk_scp *scp)
+{
+	writel(1, scp->reg_base + MT8195_CORE1_SW_RSTN_CLR);
+}
+
 static void mt8183_scp_irq_handler(struct mtk_scp *scp)
 {
 	u32 scp_to_host;
@@ -241,6 +251,24 @@ static void mt8195_scp_irq_handler(struct mtk_scp *scp)
 	}
 }
 
+static void mt8195_scp_dual_irq_handler(struct mtk_scp *scp)
+{
+	u32 scp_to_host;
+
+	scp_to_host = readl(scp->reg_base + MT8195_SSHUB2APMCU_IPC_SET);
+
+	if (scp_to_host & MT8192_SCP_IPC_INT_BIT) {
+		scp_ipi_handler(scp);
+
+		/*
+		 * SCP won't send another interrupt until we clear
+		 * MT8195_SSHUB2APMCU_IPC_CLR.
+		 */
+		writel(MT8192_SCP_IPC_INT_BIT,
+		       scp->reg_base + MT8195_SSHUB2APMCU_IPC_CLR);
+	}
+}
+
 static irqreturn_t scp_irq_handler(int irq, void *priv)
 {
 	struct mtk_scp *scp = priv;
@@ -474,6 +502,21 @@ static int mt8195_scp_before_load(struct mtk_scp *scp)
 	return 0;
 }
 
+static int mt8195_scp_dual_before_load(struct mtk_scp *scp)
+{
+	u32 sec_ctrl;
+
+	scp_sram_power_on(scp->reg_base + MT8195_CPU1_SRAM_PD, 0);
+
+	/* hold SCP in reset while loading FW. */
+	scp->data->scp_reset_assert(scp);
+
+	/* enable MPU for all memory regions */
+	writel(0xff, scp->reg_base + MT8195_CORE1_MEM_ATT_PREDEF);
+
+	return 0;
+}
+
 static int scp_load(struct rproc *rproc, const struct firmware *fw)
 {
 	struct mtk_scp *scp = rproc->priv;
@@ -646,6 +689,15 @@ static void mt8195_scp_stop(struct mtk_scp *scp)
 	writel(0, scp->reg_base + MT8192_CORE0_WDT_CFG);
 }
 
+static void mt8195_scp_dual_stop(struct mtk_scp *scp)
+{
+	/* Power off CPU SRAM */
+	scp_sram_power_off(scp->reg_base + MT8195_CPU1_SRAM_PD, 0);
+
+	/* Disable SCP watchdog */
+	writel(0, scp->reg_base + MT8195_CORE1_WDT_CFG);
+}
+
 static int scp_stop(struct rproc *rproc)
 {
 	struct mtk_scp *scp = (struct mtk_scp *)rproc->priv;
@@ -1013,11 +1065,24 @@ static const struct mtk_scp_of_data mt8195_of_data = {
 	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
 };
 
+static const struct mtk_scp_of_data mt8195_scp_dual_of_data = {
+	.scp_clk_get = mt8195_scp_clk_get,
+	.scp_before_load = mt8195_scp_dual_before_load,
+	.scp_irq_handler = mt8195_scp_dual_irq_handler,
+	.scp_reset_assert = mt8195_scp_dual_reset_assert,
+	.scp_reset_deassert = mt8195_scp_dual_reset_deassert,
+	.scp_stop = mt8195_scp_dual_stop,
+	.scp_da_to_va = mt8192_scp_da_to_va,
+	.host_to_scp_reg = MT8192_GIPC_IN_SET,
+	.host_to_scp_int_bit = MT8195_CORE1_HOST_IPC_INT_BIT,
+};
+
 static const struct of_device_id mtk_scp_of_match[] = {
 	{ .compatible = "mediatek,mt8183-scp", .data = &mt8183_of_data },
 	{ .compatible = "mediatek,mt8186-scp", .data = &mt8186_of_data },
 	{ .compatible = "mediatek,mt8192-scp", .data = &mt8192_of_data },
 	{ .compatible = "mediatek,mt8195-scp", .data = &mt8195_of_data },
+	{ .compatible = "mediatek,mt8195-scp-dual", .data = &mt8195_scp_dual_of_data },
 	{},
 };
 MODULE_DEVICE_TABLE(of, mtk_scp_of_match);
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/9] remoteproc: mediatek: Add SCP core 1 SRAM offset

[Tinghan Shen]

Because SCP core 0 and core 1 both boot from address 0 and have the same
viewpoint of memory, HW has a set of registers, "SRAM offset", to add
offset to accessed address for SCP core 1 to solve this problem.

The "SRAM offset" configuration is composed by specifying a range and an
offset. The value of range is from the viewpoint of SCP core 1.
When SCP core 1 accessing addresses in the configured range, SCP bus
adds an offset to shift the destination on SCP SRAM. This shift is
transparent to the software running on SCP core 1.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
---
 drivers/remoteproc/mtk_scp.c | 42 ++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index 731a8094c373..b8a4db581179 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -505,6 +505,27 @@ static int mt8195_scp_before_load(struct mtk_scp *scp)
 static int mt8195_scp_dual_before_load(struct mtk_scp *scp)
 {
 	u32 sec_ctrl;
+	struct device *dev = scp->dev;
+	struct device_node *main_np;
+	struct platform_device *main_pdev;
+	struct mtk_scp *scp_core0;
+
+	/* Get sram start address from SCP core 0 */
+	main_np = of_parse_phandle(dev->of_node, "mediatek,scp-core", 0);
+	if (!main_np) {
+		dev_warn(dev, "Invalid SCP main core phandle\n");
+		return -EINVAL;
+	}
+
+	main_pdev = of_find_device_by_node(main_np);
+	of_node_put(main_np);
+
+	if (!main_pdev) {
+		dev_err(dev, "Cannot find SCP core 0 device\n");
+		return -ENODEV;
+	}
+	scp_core0 = platform_get_drvdata(main_pdev);
+	put_device(&main_pdev->dev);
 
 	scp_sram_power_on(scp->reg_base + MT8195_CPU1_SRAM_PD, 0);
 
@@ -514,6 +535,27 @@ static int mt8195_scp_dual_before_load(struct mtk_scp *scp)
 	/* enable MPU for all memory regions */
 	writel(0xff, scp->reg_base + MT8195_CORE1_MEM_ATT_PREDEF);
 
+	/* The value of SRAM offset range is from the viewpoint of SCP core 1.
+	 * This configuration adds an offset on SCP bus when SCP core 1 accesses SCP SRAM
+	 * to solve the SCP core 0 and core 1 both fetch the 1st instruction from the same
+	 * SRAM address.
+	 *
+	 * Because SCP core 0 and core 1 both boot from address 0, this must be configured
+	 * before boot SCP core 1.
+	 *
+	 * Configure the range of SRAM addresses will be added offset.
+	 */
+	writel(0, scp->reg_base + MT8195_L2TCM_OFFSET_RANGE_0_LOW);
+	writel(scp->sram_size, scp->reg_base + MT8195_L2TCM_OFFSET_RANGE_0_HIGH);
+
+	/* configure the offset value */
+	writel(scp->sram_phys - scp_core0->sram_phys, scp->reg_base + MT8195_L2TCM_OFFSET);
+
+	/* enable adding sram offset when fetching instruction and data */
+	sec_ctrl = readl(scp->reg_base + MT8195_SEC_CTRL);
+	sec_ctrl |= MT8195_CORE_OFFSET_ENABLE_I | MT8195_CORE_OFFSET_ENABLE_D;
+	writel(sec_ctrl, scp->reg_base + MT8195_SEC_CTRL);
+
 	return 0;
 }
 
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 7/9] remoteproc: mediatek: Add SCP core 1 as a rproc subdevice

[Tinghan Shen]

Because the clock and SRAM power is controlled by SCP core 0, SCP core 1
has to be boot after SCP core 0. We use the rproc subdev to achieve this
purpose.

The watchdog timeout handler of SCP core 1 is added as part of the rproc
subdevice. This allows SCP core 0 to handle watchdog timeout coming from
SCP core 1.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
---
 drivers/remoteproc/mtk_common.h |   9 +++
 drivers/remoteproc/mtk_scp.c    | 130 ++++++++++++++++++++++++++++++++
 2 files changed, 139 insertions(+)

diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
index 5582f4207fbf..67b41866a100 100644
--- a/drivers/remoteproc/mtk_common.h
+++ b/drivers/remoteproc/mtk_common.h
@@ -99,6 +99,14 @@ struct scp_ipi_desc {
 	void *priv;
 };
 
+struct scp_subdev_core {
+	struct rproc_subdev subdev;
+	struct mtk_scp *main_scp;
+	void (*scp_dual_wdt_timeout)(struct mtk_scp *scp, u32 scp_to_host);
+};
+
+#define to_subdev_core(d) container_of(d, struct scp_subdev_core, subdev)
+
 struct mtk_scp;
 
 struct mtk_scp_of_data {
@@ -144,6 +152,7 @@ struct mtk_scp {
 	size_t dram_size;
 
 	struct rproc_subdev *rpmsg_subdev;
+	struct rproc_subdev *dual_subdev;
 };
 
 /**
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index b8a4db581179..d0e9e19e251f 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -244,6 +244,13 @@ static void mt8195_scp_irq_handler(struct mtk_scp *scp)
 	} else {
 		if (readl(scp->reg_base + MT8195_SYS_STATUS) & MT8195_CORE1_WDT) {
 			writel(1, scp->reg_base + MT8195_CORE1_WDT_IRQ);
+
+			if (scp->dual_subdev) {
+				struct scp_subdev_core *subdev_core;
+
+				subdev_core = to_subdev_core(scp->dual_subdev);
+				subdev_core->scp_dual_wdt_timeout(scp, scp_to_host);
+			}
 		} else {
 			writel(1, scp->reg_base + MT8192_CORE0_WDT_IRQ);
 			scp_wdt_handler(scp, scp_to_host);
@@ -925,6 +932,118 @@ static void scp_remove_rpmsg_subdev(struct mtk_scp *scp)
 	}
 }
 
+static struct mtk_scp *scp_dual_get(struct mtk_scp *scp)
+{
+	struct device *dev = scp->dev;
+	struct device_node *np;
+	struct platform_device *dual_pdev;
+
+	np = of_parse_phandle(dev->of_node, "mediatek,scp-core", 0);
+	dual_pdev = of_find_device_by_node(np);
+	of_node_put(np);
+
+	if (!dual_pdev) {
+		dev_err(dev, "No scp-dual pdev\n");
+		return NULL;
+	}
+
+	return platform_get_drvdata(dual_pdev);
+}
+
+static void scp_dual_put(struct mtk_scp *scp)
+{
+	put_device(scp->dev);
+}
+
+static int scp_dual_rproc_start(struct rproc_subdev *subdev)
+{
+	struct scp_subdev_core *subdev_core = to_subdev_core(subdev);
+	struct mtk_scp *scp_dual;
+
+	scp_dual = scp_dual_get(subdev_core->main_scp);
+	if (!scp_dual)
+		return -ENODEV;
+
+	rproc_boot(scp_dual->rproc);
+	scp_dual_put(scp_dual);
+
+	return 0;
+}
+
+static void scp_dual_rproc_stop(struct rproc_subdev *subdev, bool crashed)
+{
+	struct scp_subdev_core *subdev_core = to_subdev_core(subdev);
+	struct mtk_scp *scp_dual;
+
+	scp_dual = scp_dual_get(subdev_core->main_scp);
+	if (!scp_dual)
+		return;
+
+	rproc_shutdown(scp_dual->rproc);
+	scp_dual_put(scp_dual);
+}
+
+static void scp_dual_wdt_handler(struct mtk_scp *scp, u32 scp_to_host)
+{
+	struct mtk_scp *scp_dual;
+
+	scp_dual = scp_dual_get(scp);
+	if (!scp_dual)
+		return;
+
+	dev_err(scp_dual->dev, "SCP watchdog timeout! 0x%x\n", scp_to_host);
+	rproc_report_crash(scp_dual->rproc, RPROC_WATCHDOG);
+	scp_dual_put(scp_dual);
+}
+
+static struct rproc_subdev *scp_dual_create_subdev(struct mtk_scp *scp)
+{
+	struct device *dev = scp->dev;
+	struct scp_subdev_core *subdev_core;
+	struct device_node *np;
+
+	np = of_parse_phandle(dev->of_node, "mediatek,scp-core", 0);
+	if (!np)
+		return NULL;
+
+	of_node_put(np);
+
+	subdev_core = devm_kzalloc(dev, sizeof(*subdev_core), GFP_KERNEL);
+	if (!subdev_core)
+		return NULL;
+
+	subdev_core->main_scp = scp;
+	subdev_core->scp_dual_wdt_timeout = scp_dual_wdt_handler;
+	subdev_core->subdev.start = scp_dual_rproc_start;
+	subdev_core->subdev.stop = scp_dual_rproc_stop;
+
+	return &subdev_core->subdev;
+}
+
+static void scp_dual_destroy_subdev(struct rproc_subdev *subdev)
+{
+	struct scp_subdev_core *subdev_core = to_subdev_core(subdev);
+
+	devm_kfree(subdev_core->main_scp->dev, subdev_core);
+}
+
+static void scp_add_dual_subdev(struct mtk_scp *scp)
+{
+	scp->dual_subdev = scp_dual_create_subdev(scp);
+
+	if (scp->dual_subdev)
+		rproc_add_subdev(scp->rproc, scp->dual_subdev);
+}
+
+static void scp_remove_dual_subdev(struct mtk_scp *scp)
+{
+	if (scp->dual_subdev) {
+		rproc_remove_subdev(scp->rproc, scp->dual_subdev);
+		scp_dual_destroy_subdev(scp->dual_subdev);
+		scp->dual_subdev = NULL;
+	}
+}
+
 static int scp_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1014,6 +1133,9 @@ static int scp_probe(struct platform_device *pdev)
 
 	scp_add_rpmsg_subdev(scp);
 
+	if (core_id == SCP_CORE_0)
+		scp_add_dual_subdev(scp);
+
 	ret = devm_request_threaded_irq(dev, platform_get_irq(pdev, 0), NULL,
 					scp_irq_handler, IRQF_ONESHOT,
 					pdev->name, scp);
@@ -1023,6 +1145,12 @@ static int scp_probe(struct platform_device *pdev)
 		goto remove_subdev;
 	}
 
+	/* disable auto boot before register rproc.
+	 * scp core 1 is booted as a subdevice of scp core 0.
+	 */
+	if (core_id == SCP_CORE_1)
+		rproc->auto_boot = false;
+
 	ret = rproc_add(rproc);
 	if (ret)
 		goto remove_subdev;
@@ -1030,6 +1158,7 @@ static int scp_probe(struct platform_device *pdev)
 	return 0;
 
 remove_subdev:
+	scp_remove_dual_subdev(scp);
 	scp_remove_rpmsg_subdev(scp);
 	scp_ipi_unregister(scp, SCP_IPI_INIT);
 release_dev_mem:
@@ -1047,6 +1176,7 @@ static int scp_remove(struct platform_device *pdev)
 	int i;
 
 	rproc_del(scp->rproc);
+	scp_remove_dual_subdev(scp);
 	scp_remove_rpmsg_subdev(scp);
 	scp_ipi_unregister(scp, SCP_IPI_INIT);
 	scp_unmap_memory_region(scp);
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 8/9] remoteproc: mediatek: Wait SCP core 1 probe done

[Tinghan Shen]

SCP core 1 driver probing must finish before start loading SCP core 1
image. Add a simple flag checking mechanism when preparing SCP core 1
subdevice.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
---
 drivers/remoteproc/mtk_common.h |  1 +
 drivers/remoteproc/mtk_scp.c    | 38 ++++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
index 67b41866a100..89f9e53a5879 100644
--- a/drivers/remoteproc/mtk_common.h
+++ b/drivers/remoteproc/mtk_common.h
@@ -153,6 +153,7 @@ struct mtk_scp {
 
 	struct rproc_subdev *rpmsg_subdev;
 	struct rproc_subdev *dual_subdev;
+	int dual_probe_done;
 };
 
 /**
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index d0e9e19e251f..b66bee4beb4d 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -937,6 +937,7 @@ static struct mtk_scp *scp_dual_get(struct mtk_scp *scp)
 	struct device *dev = scp->dev;
 	struct device_node *np;
 	struct platform_device *dual_pdev;
+	struct mtk_scp *scp_dual;
 
 	np = of_parse_phandle(dev->of_node, "mediatek,scp-core", 0);
 	dual_pdev = of_find_device_by_node(np);
@@ -947,7 +948,11 @@ static struct mtk_scp *scp_dual_get(struct mtk_scp *scp)
 		return NULL;
 	}
 
-	return platform_get_drvdata(dual_pdev);
+	scp_dual = platform_get_drvdata(dual_pdev);
+	if (!scp_dual)
+		put_device(&dual_pdev->dev);
+
+	return scp_dual;
 }
 
 static void scp_dual_put(struct mtk_scp *scp)
@@ -955,6 +960,33 @@ static void scp_dual_put(struct mtk_scp *scp)
 	put_device(scp->dev);
 }
 
+static int scp_dual_rproc_prepare(struct rproc_subdev *subdev)
+{
+	struct scp_subdev_core *subdev_core = to_subdev_core(subdev);
+	struct mtk_scp *scp = subdev_core->main_scp;
+	unsigned long timeout;
+
+	timeout = jiffies + msecs_to_jiffies(100);
+	while (1) {
+		struct mtk_scp *scp_dual = scp_dual_get(scp);
+
+		if (scp_dual && scp_dual->dual_probe_done) {
+			scp_dual_put(scp_dual);
+			break;
+		}
+
+		if (scp_dual && !scp_dual->dual_probe_done)
+			scp_dual_put(scp_dual);
+
+		if (time_after(jiffies, timeout)) {
+			dev_err(scp->dev, "scp-dual probe timeout\n");
+			return -ETIMEDOUT;
+		}
+	}
+
+	return 0;
+}
+
 static int scp_dual_rproc_start(struct rproc_subdev *subdev)
 {
 	struct scp_subdev_core *subdev_core = to_subdev_core(subdev);
@@ -1014,6 +1046,7 @@ static struct rproc_subdev *scp_dual_create_subdev(struct mtk_scp *scp)
 
 	subdev_core->main_scp = scp;
 	subdev_core->scp_dual_wdt_timeout = scp_dual_wdt_handler;
+	subdev_core->subdev.prepare = scp_dual_rproc_prepare;
 	subdev_core->subdev.start = scp_dual_rproc_start;
 	subdev_core->subdev.stop = scp_dual_rproc_stop;
 
@@ -1155,6 +1188,9 @@ static int scp_probe(struct platform_device *pdev)
 	if (ret)
 		goto remove_subdev;
 
+	if (core_id == SCP_CORE_1)
+		scp->dual_probe_done = 1;
+
 	return 0;
 
 remove_subdev:
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 9/9] mfd: cros_ec: Add SCP core 1 as a new CrOS EC MCU

[Tinghan Shen]

MT8195 System Companion Processors(SCP) is a dual-core RISC-V MCU.
Add a new cros feature id to represent the SCP 2nd core.

The 1st core is referred to as 'core 0', and the 2nd core is referred
to as 'core 1'.

Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
---
 drivers/mfd/cros_ec_dev.c                      | 5 +++++
 include/linux/platform_data/cros_ec_commands.h | 2 ++
 include/linux/platform_data/cros_ec_proto.h    | 1 +
 3 files changed, 8 insertions(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 546feef851ab..7be2e23525e1 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -64,6 +64,11 @@ static const struct cros_feature_to_name cros_mcu_devices[] = {
 		.name	= CROS_EC_DEV_SCP_NAME,
 		.desc	= "System Control Processor",
 	},
+	{
+		.id	= EC_FEATURE_SCP_C1,
+		.name	= CROS_EC_DEV_SCP_C1_NAME,
+		.desc	= "System Control Processor 2nd Core",
+	},
 	{
 		.id	= EC_FEATURE_TOUCHPAD,
 		.name	= CROS_EC_DEV_TP_NAME,
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index c23554531961..1176cdc92d23 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -1296,6 +1296,8 @@ enum ec_feature_code {
 	 * mux.
 	 */
 	EC_FEATURE_TYPEC_MUX_REQUIRE_AP_ACK = 43,
+	/* The MCU is a System Companion Processor (SCP) 2nd Core. */
+	EC_FEATURE_SCP_C1 = 45,
 };
 
 #define EC_FEATURE_MASK_0(event_code) BIT(event_code % 32)
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index df3c78c92ca2..12fc60f3c90d 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -19,6 +19,7 @@
 #define CROS_EC_DEV_ISH_NAME	"cros_ish"
 #define CROS_EC_DEV_PD_NAME	"cros_pd"
 #define CROS_EC_DEV_SCP_NAME	"cros_scp"
+#define CROS_EC_DEV_SCP_C1_NAME	"cros_scp_c1"
 #define CROS_EC_DEV_TP_NAME	"cros_tp"
 
 /*
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/9] Add support for MT8195 SCP 2nd core

[Tinghan Shen]

On Wed, 2022-06-08 at 16:35 +0800, Tinghan Shen wrote:
> The mediatek remoteproc driver currently only allows bringing up a 
> single core SCP, e.g. MT8183. It also only bringing up the 1st 
> core in SoCs with a dual-core SCP, e.g. MT8195. This series support 
> to bring-up the 2nd core of the dual-core SCP.

Add missing change log.

v1 -> v2:
1. update dt-binding property description
2. remove kconfig for scp dual driver
3. merge mtk_scp_dual.c and mtk_scp_subdev.c to mtk_scp.c

Regards,
TingHan

> 
> Tinghan Shen (9):
>   dt-binding: remoteproc: mediatek: Support dual-core SCP
>   remoteproc: mediatek: Support hanlding scp core 1 wdt timeout
>   remoteproc: mediatek: Add SCP core 1 register definitions
>   remoteproc: mediatek: Support probing for the 2nd core of dual-core
>     SCP
>   remoteproc: mediatek: Add chip dependent operations for SCP core 1
>   remoteproc: mediatek: Add SCP core 1 SRAM offset
>   remoteproc: mediatek: Add SCP core 1 as a rproc subdevice
>   remoteproc: mediatek: Wait SCP core 1 probe done
>   mfd: cros_ec: Add SCP core 1 as a new CrOS EC MCU
> 
>  .../bindings/remoteproc/mtk,scp.yaml          |  13 +
>  drivers/mfd/cros_ec_dev.c                     |   5 +
>  drivers/remoteproc/mtk_common.h               |  35 ++
>  drivers/remoteproc/mtk_scp.c                  | 322 +++++++++++++++++-
>  .../linux/platform_data/cros_ec_commands.h    |   2 +
>  include/linux/platform_data/cros_ec_proto.h   |   1 +
>  6 files changed, 375 insertions(+), 3 deletions(-)
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/9] dt-binding: remoteproc: mediatek: Support dual-core SCP

[Rob Herring]

On Wed, 08 Jun 2022 16:35:45 +0800, Tinghan Shen wrote:
> The MT8195 SCP co-processor is a dual-core RISC-V MCU.
> 
> Add a new property to reference the sibling core and to assign
> core id to SCP nodes. Also add a new compatile for the driver of
> SCP 2nd core.
> 
> Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> ---
>  .../devicetree/bindings/remoteproc/mtk,scp.yaml     | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/9] remoteproc: mediatek: Support hanlding scp core 1 wdt timeout

[Mathieu Poirier]

Hi Tinghan,

I have started reviewing this set and I expect comments to be spread out over a few
days.  I will tell you when I am done.

Please see below for comments...

On Wed, Jun 08, 2022 at 04:35:46PM +0800, Tinghan Shen wrote:
> MT8195 SCP is a dual-core processor. The SCP core 1 watchdog timeout
> interrupt uses the same interrupt line of SCP core 0 watchdog timeout
> interrupt.
> 
> Add support for handling SCP core 1 watchdog timeout interrupt in the
> SCP IRQ handler.
> 
> Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> ---
>  drivers/remoteproc/mtk_common.h |  4 ++++
>  drivers/remoteproc/mtk_scp.c    | 27 ++++++++++++++++++++++++++-
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
> index ea6fa1100a00..73e8adf00de3 100644
> --- a/drivers/remoteproc/mtk_common.h
> +++ b/drivers/remoteproc/mtk_common.h
> @@ -54,6 +54,10 @@
>  #define MT8192_CORE0_WDT_IRQ		0x10030
>  #define MT8192_CORE0_WDT_CFG		0x10034
>  
> +#define MT8195_SYS_STATUS		0x4004
> +#define MT8195_CORE0_WDT		BIT(16)
> +#define MT8195_CORE1_WDT		BIT(17)
> +
>  #define MT8195_L1TCM_SRAM_PDN_RESERVED_RSI_BITS		GENMASK(7, 4)
>  
>  #define SCP_FW_VER_LEN			32
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index 47b2a40e1b4a..3510c6d0bbc8 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -212,6 +212,31 @@ static void mt8192_scp_irq_handler(struct mtk_scp *scp)
>  	}
>  }
>  
> +static void mt8195_scp_irq_handler(struct mtk_scp *scp)
> +{
> +	u32 scp_to_host;
> +
> +	scp_to_host = readl(scp->reg_base + MT8192_SCP2APMCU_IPC_SET);
> +
> +	if (scp_to_host & MT8192_SCP_IPC_INT_BIT) {
> +		scp_ipi_handler(scp);
> +
> +		/*
> +		 * SCP won't send another interrupt until we clear
> +		 * MT8192_SCP2APMCU_IPC.
> +		 */
> +		writel(MT8192_SCP_IPC_INT_BIT,
> +		       scp->reg_base + MT8192_SCP2APMCU_IPC_CLR);
> +	} else {
> +		if (readl(scp->reg_base + MT8195_SYS_STATUS) & MT8195_CORE1_WDT) {
> +			writel(1, scp->reg_base + MT8195_CORE1_WDT_IRQ);
> +		} else {
> +			writel(1, scp->reg_base + MT8192_CORE0_WDT_IRQ);
> +			scp_wdt_handler(scp, scp_to_host);

Why is scp_wdt_handler() not called when CORE1 signals a watchdog failure?  If
this is the intended behaviour there is no way for anyone but you to know that
it is the case.  

> +		}
> +	}
> +}
> +
>  static irqreturn_t scp_irq_handler(int irq, void *priv)
>  {
>  	struct mtk_scp *scp = priv;
> @@ -961,7 +986,7 @@ static const struct mtk_scp_of_data mt8192_of_data = {
>  static const struct mtk_scp_of_data mt8195_of_data = {
>  	.scp_clk_get = mt8195_scp_clk_get,
>  	.scp_before_load = mt8195_scp_before_load,
> -	.scp_irq_handler = mt8192_scp_irq_handler,
> +	.scp_irq_handler = mt8195_scp_irq_handler,
>  	.scp_reset_assert = mt8192_scp_reset_assert,
>  	.scp_reset_deassert = mt8192_scp_reset_deassert,
>  	.scp_stop = mt8195_scp_stop,
> -- 
> 2.18.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/9] remoteproc: mediatek: Add SCP core 1 register definitions

[Mathieu Poirier]

On Wed, Jun 08, 2022 at 04:35:47PM +0800, Tinghan Shen wrote:
> Add MT8195 SCP core 1 related register definitions.
> 
> Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> ---
>  drivers/remoteproc/mtk_common.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
> index 73e8adf00de3..5582f4207fbf 100644
> --- a/drivers/remoteproc/mtk_common.h
> +++ b/drivers/remoteproc/mtk_common.h
> @@ -47,6 +47,7 @@
>  #define MT8192_SCP2SPM_IPC_CLR		0x4094
>  #define MT8192_GIPC_IN_SET		0x4098
>  #define MT8192_HOST_IPC_INT_BIT		BIT(0)
> +#define MT8195_CORE1_HOST_IPC_INT_BIT	BIT(4)
>  
>  #define MT8192_CORE0_SW_RSTN_CLR	0x10000
>  #define MT8192_CORE0_SW_RSTN_SET	0x10004
> @@ -60,6 +61,26 @@
>  
>  #define MT8195_L1TCM_SRAM_PDN_RESERVED_RSI_BITS		GENMASK(7, 4)
>  
> +#define MT8195_CPU1_SRAM_PD			0x1084
> +#define MT8195_SSHUB2APMCU_IPC_SET		0x4088
> +#define MT8195_SSHUB2APMCU_IPC_CLR		0x408C
> +#define MT8195_CORE1_SW_RSTN_CLR		0x20000
> +#define MT8195_CORE1_SW_RSTN_SET		0x20004
> +#define MT8195_CORE1_MEM_ATT_PREDEF		0x20008
> +#define MT8195_CORE1_WDT_IRQ			0x20030
> +#define MT8195_CORE1_WDT_CFG			0x20034
> +
> +#define MT8195_SEC_CTRL				0x85000
> +#define MT8195_CORE_OFFSET_ENABLE_D		BIT(13)
> +#define MT8195_CORE_OFFSET_ENABLE_I		BIT(12)
> +#define MT8195_L2TCM_OFFSET_RANGE_0_LOW		0x850b0
> +#define MT8195_L2TCM_OFFSET_RANGE_0_HIGH	0x850b4
> +#define MT8195_L2TCM_OFFSET			0x850d0
> +#define SCP_SRAM_REMAP_LOW			0
> +#define SCP_SRAM_REMAP_HIGH			1
> +#define SCP_SRAM_REMAP_OFFSET			2
> +#define SCP_SRAM_REMAP_SIZE			3
> +
>  #define SCP_FW_VER_LEN			32
>  #define SCP_SHARE_BUFFER_SIZE		288

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

>  
> -- 
> 2.18.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP

[Mathieu Poirier]

On Wed, Jun 08, 2022 at 04:35:48PM +0800, Tinghan Shen wrote:
> The mtk_scp.c driver only supports the single core SCP and the
> 1st core of a dual-core SCP. This patch extends it for the 2nd core.
> 
> MT8195 SCP is a dual-core MCU. Both cores are housed in the same subsys.

s/subsys/subsystem

> They have the same viewpoint of registers and memory.
> 
> Core 1 of the SCP features its own set of core configuration registers,
> interrupt controller, timers, and DMAs. The rest of the peripherals
> in this subsystem are shared by core 0 and core 1.
> 
> As for memory, core 1 has its own cache memory. the SCP SRAM is shared

/the/The

> by core 0 and core 1.
> 
> Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> ---
>  drivers/remoteproc/mtk_scp.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index 3510c6d0bbc8..91b4aefde4ac 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -23,6 +23,10 @@
>  #define MAX_CODE_SIZE 0x500000
>  #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
>  
> +#define SCP_CORE_0 0
> +#define SCP_CORE_1 1
> +#define SCP_CORE_SINGLE 0xF
> +
>  /**
>   * scp_get() - get a reference to SCP.
>   *
> @@ -836,6 +840,7 @@ static int scp_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	const char *fw_name = "scp.img";
>  	int ret, i;
> +	u32 core_id = SCP_CORE_SINGLE;
>  
>  	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
>  	if (ret < 0 && ret != -EINVAL)
> @@ -851,8 +856,16 @@ static int scp_probe(struct platform_device *pdev)
>  	scp->data = of_device_get_match_data(dev);
>  	platform_set_drvdata(pdev, scp);
>  
> +	ret = of_property_read_u32_index(dev->of_node, "mediatek,scp-core", 1, &core_id);
> +	if (ret == 0)
> +		dev_info(dev, "Boot SCP dual core %u\n", core_id);

Why is the DT property "mediatek,scp-core" needed at all?  Since the compatible
"mediatek,mt8195-scp-dual" has already been defined previously in this patchset,
initialising the second core, if present, is a matter of looking at the
compatile string. 

> +
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
> -	scp->sram_base = devm_ioremap_resource(dev, res);
> +	if (core_id == SCP_CORE_1)
> +		scp->sram_base = devm_ioremap(dev, res->start, resource_size(res));
> +	else
> +		scp->sram_base = devm_ioremap_resource(dev, res);
> +

This looks very broken...  For this to work you would need to have two DT
entries with the "mediatek,mt8195-scp-dual" compatible properly, one with
"mediatek,scp-core = <&scp_dual1 0>;" and another one with "mediatek,scp-core = <&scp_dual0 1>;".

Which is also very broken...  Here you have a binding whose first argument is a
reference to the core sibling while the second argument is a characteristic of
the current core, which is highly confusing.

I suggest what when you see the compatible binding "mediatek,mt8195-scp", a
single core is initialized.  If you see "mediatek,mt8195-scp-dual", both cores
are initialized as part of the _same_ probe.

If the above analysis is not correct it means I misinterpreted your
work and if so, a serious amount of comments is needed _and_ a very detailed
example in "mtk,scp.yaml" that leaves no room for interpretation.

I will stop reviewing this patchset until you have clarified how this works.

Thanks,
Mathieu

>  	if (IS_ERR(scp->sram_base))
>  		return dev_err_probe(dev, PTR_ERR(scp->sram_base),
>  				     "Failed to parse and map sram memory\n");
> @@ -873,7 +886,12 @@ static int scp_probe(struct platform_device *pdev)
>  		scp->l1tcm_phys = res->start;
>  	}
>  
> -	scp->reg_base = devm_platform_ioremap_resource_byname(pdev, "cfg");
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
> +	if (core_id == SCP_CORE_1)
> +		scp->reg_base = devm_ioremap(dev, res->start, resource_size(res));
> +	else
> +		scp->reg_base = devm_ioremap_resource(dev, res);
> +
>  	if (IS_ERR(scp->reg_base))
>  		return dev_err_probe(dev, PTR_ERR(scp->reg_base),
>  				     "Failed to parse and map cfg memory\n");
> -- 
> 2.18.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/9] remoteproc: mediatek: Support hanlding scp core 1 wdt timeout

[Tinghan Shen]

Hi Mathieu,

> Hi Tinghan,
> 
> I have started reviewing this set and I expect comments to be spread out over a few
> days.  I will tell you when I am done.
> 
> Please see below for comments...
> 
> On Wed, Jun 08, 2022 at 04:35:46PM +0800, Tinghan Shen wrote:
> > MT8195 SCP is a dual-core processor. The SCP core 1 watchdog timeout
> > interrupt uses the same interrupt line of SCP core 0 watchdog timeout
> > interrupt.
> > 
> > Add support for handling SCP core 1 watchdog timeout interrupt in the
> > SCP IRQ handler.
> > 
> > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > ---
> >  drivers/remoteproc/mtk_common.h |  4 ++++
> >  drivers/remoteproc/mtk_scp.c    | 27 ++++++++++++++++++++++++++-
> >  2 files changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
> > index ea6fa1100a00..73e8adf00de3 100644
> > --- a/drivers/remoteproc/mtk_common.h
> > +++ b/drivers/remoteproc/mtk_common.h
> > @@ -54,6 +54,10 @@
> >  #define MT8192_CORE0_WDT_IRQ		0x10030
> >  #define MT8192_CORE0_WDT_CFG		0x10034
> >  
> > +#define MT8195_SYS_STATUS		0x4004
> > +#define MT8195_CORE0_WDT		BIT(16)
> > +#define MT8195_CORE1_WDT		BIT(17)
> > +
> >  #define MT8195_L1TCM_SRAM_PDN_RESERVED_RSI_BITS		GENMASK(7, 4)
> >  
> >  #define SCP_FW_VER_LEN			32
> > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> > index 47b2a40e1b4a..3510c6d0bbc8 100644
> > --- a/drivers/remoteproc/mtk_scp.c
> > +++ b/drivers/remoteproc/mtk_scp.c
> > @@ -212,6 +212,31 @@ static void mt8192_scp_irq_handler(struct mtk_scp *scp)
> >  	}
> >  }
> >  
> > +static void mt8195_scp_irq_handler(struct mtk_scp *scp)
> > +{
> > +	u32 scp_to_host;
> > +
> > +	scp_to_host = readl(scp->reg_base + MT8192_SCP2APMCU_IPC_SET);
> > +
> > +	if (scp_to_host & MT8192_SCP_IPC_INT_BIT) {
> > +		scp_ipi_handler(scp);
> > +
> > +		/*
> > +		 * SCP won't send another interrupt until we clear
> > +		 * MT8192_SCP2APMCU_IPC.
> > +		 */
> > +		writel(MT8192_SCP_IPC_INT_BIT,
> > +		       scp->reg_base + MT8192_SCP2APMCU_IPC_CLR);
> > +	} else {
> > +		if (readl(scp->reg_base + MT8195_SYS_STATUS) & MT8195_CORE1_WDT) {
> > +			writel(1, scp->reg_base + MT8195_CORE1_WDT_IRQ);
> > +		} else {
> > +			writel(1, scp->reg_base + MT8192_CORE0_WDT_IRQ);
> > +			scp_wdt_handler(scp, scp_to_host);
> 
> Why is scp_wdt_handler() not called when CORE1 signals a watchdog failure?  If
> this is the intended behaviour there is no way for anyone but you to know that
> it is the case.  

It's becuase the handler of CORE1 timeout doesn't exist at this patch.
The CORE1 timeout handler is added at patch 7 of this series.

You're right. This makes people confused.
I'll combine this patch with the CORE1 timeout handler.

Best regards,
TingHan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP

[Tinghan Shen]

Hi Mathieu,

> > The mtk_scp.c driver only supports the single core SCP and the
> > 1st core of a dual-core SCP. This patch extends it for the 2nd core.
> > 
> > MT8195 SCP is a dual-core MCU. Both cores are housed in the same subsys.
> 
> s/subsys/subsystem
> 
> > They have the same viewpoint of registers and memory.
> > 
> > Core 1 of the SCP features its own set of core configuration registers,
> > interrupt controller, timers, and DMAs. The rest of the peripherals
> > in this subsystem are shared by core 0 and core 1.
> > 
> > As for memory, core 1 has its own cache memory. the SCP SRAM is shared
> 
> /the/The
> 
> > by core 0 and core 1.
> > 
> > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > ---
> >  drivers/remoteproc/mtk_scp.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> > index 3510c6d0bbc8..91b4aefde4ac 100644
> > --- a/drivers/remoteproc/mtk_scp.c
> > +++ b/drivers/remoteproc/mtk_scp.c
> > @@ -23,6 +23,10 @@
> >  #define MAX_CODE_SIZE 0x500000
> >  #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
> >  
> > +#define SCP_CORE_0 0
> > +#define SCP_CORE_1 1
> > +#define SCP_CORE_SINGLE 0xF
> > +
> >  /**
> >   * scp_get() - get a reference to SCP.
> >   *
> > @@ -836,6 +840,7 @@ static int scp_probe(struct platform_device *pdev)
> >  	struct resource *res;
> >  	const char *fw_name = "scp.img";
> >  	int ret, i;
> > +	u32 core_id = SCP_CORE_SINGLE;
> >  
> >  	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> >  	if (ret < 0 && ret != -EINVAL)
> > @@ -851,8 +856,16 @@ static int scp_probe(struct platform_device *pdev)
> >  	scp->data = of_device_get_match_data(dev);
> >  	platform_set_drvdata(pdev, scp);
> >  
> > +	ret = of_property_read_u32_index(dev->of_node, "mediatek,scp-core", 1, &core_id);
> > +	if (ret == 0)
> > +		dev_info(dev, "Boot SCP dual core %u\n", core_id);
> 
> Why is the DT property "mediatek,scp-core" needed at all?  Since the compatible
> "mediatek,mt8195-scp-dual" has already been defined previously in this patchset,
> initialising the second core, if present, is a matter of looking at the
> compatile string. 

This idea of identify cores by the compatible looks workable.
I'll update this series at next version.
Thanks!

> 
> > +
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
> > -	scp->sram_base = devm_ioremap_resource(dev, res);
> > +	if (core_id == SCP_CORE_1)
> > +		scp->sram_base = devm_ioremap(dev, res->start, resource_size(res));
> > +	else
> > +		scp->sram_base = devm_ioremap_resource(dev, res);
> > +
> 
> This looks very broken...  For this to work you would need to have two DT
> entries with the "mediatek,mt8195-scp-dual" compatible properly, one with
> "mediatek,scp-core = <&scp_dual1 0>;" and another one with "mediatek,scp-core = <&scp_dual0 1>;".
> 
> Which is also very broken...  Here you have a binding whose first argument is a
> reference to the core sibling while the second argument is a characteristic of
> the current core, which is highly confusing.
> 
> I suggest what when you see the compatible binding "mediatek,mt8195-scp", a
> single core is initialized.  If you see "mediatek,mt8195-scp-dual", both cores
> are initialized as part of the _same_ probe.
> 
> If the above analysis is not correct it means I misinterpreted your
> work and if so, a serious amount of comments is needed _and_ a very detailed
> example in "mtk,scp.yaml" that leaves no room for interpretation.
> 
> I will stop reviewing this patchset until you have clarified how this works.
> 
> Thanks,
> Mathieu

There's one problem of initializng the CORE1 using the same probe flow.
The register space of CORE0 and CORE1 are overlapped in the device node.
Both cores need to use the 'cfg' registers defined in scp yaml. 
The devm_ioremap_resource catches address overlapping and returns error when 
probing CORE1 driver.


Best regards,
TingHan


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP

[TingHan Shen]

On Thu, 2022-09-08 at 14:58 -0600, Mathieu Poirier wrote:
> On Thu, 8 Sept 2022 at 05:21, Tinghan Shen <tinghan.shen@mediatek.com>
> wrote:
> 
> > Hi Mathieu,
> > 
> > > > The mtk_scp.c driver only supports the single core SCP and the
> > > > 1st core of a dual-core SCP. This patch extends it for the 2nd core.
> > > > 
> > > > MT8195 SCP is a dual-core MCU. Both cores are housed in the same
> > 
> > subsys.
> > > 
> > > s/subsys/subsystem
> > > 
> > > > They have the same viewpoint of registers and memory.
> > > > 
> > > > Core 1 of the SCP features its own set of core configuration registers,
> > > > interrupt controller, timers, and DMAs. The rest of the peripherals
> > > > in this subsystem are shared by core 0 and core 1.
> > > > 
> > > > As for memory, core 1 has its own cache memory. the SCP SRAM is shared
> > > 
> > > /the/The
> > > 
> > > > by core 0 and core 1.
> > > > 
> > > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > > > ---
> > > >  drivers/remoteproc/mtk_scp.c | 22 ++++++++++++++++++++--
> > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/remoteproc/mtk_scp.c
> > 
> > b/drivers/remoteproc/mtk_scp.c
> > > > index 3510c6d0bbc8..91b4aefde4ac 100644
> > > > --- a/drivers/remoteproc/mtk_scp.c
> > > > +++ b/drivers/remoteproc/mtk_scp.c
> > > > @@ -23,6 +23,10 @@
> > > >  #define MAX_CODE_SIZE 0x500000
> > > >  #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
> > > > 
> > > > +#define SCP_CORE_0 0
> > > > +#define SCP_CORE_1 1
> > > > +#define SCP_CORE_SINGLE 0xF
> > > > +
> > > >  /**
> > > >   * scp_get() - get a reference to SCP.
> > > >   *
> > > > @@ -836,6 +840,7 @@ static int scp_probe(struct platform_device *pdev)
> > > >     struct resource *res;
> > > >     const char *fw_name = "scp.img";
> > > >     int ret, i;
> > > > +   u32 core_id = SCP_CORE_SINGLE;
> > > > 
> > > >     ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> > > >     if (ret < 0 && ret != -EINVAL)
> > > > @@ -851,8 +856,16 @@ static int scp_probe(struct platform_device *pdev)
> > > >     scp->data = of_device_get_match_data(dev);
> > > >     platform_set_drvdata(pdev, scp);
> > > > 
> > > > +   ret = of_property_read_u32_index(dev->of_node,
> > 
> > "mediatek,scp-core", 1, &core_id);
> > > > +   if (ret == 0)
> > > > +           dev_info(dev, "Boot SCP dual core %u\n", core_id);
> > > 
> > > Why is the DT property "mediatek,scp-core" needed at all?  Since the
> > 
> > compatible
> > > "mediatek,mt8195-scp-dual" has already been defined previously in this
> > 
> > patchset,
> > > initialising the second core, if present, is a matter of looking at the
> > > compatile string.
> > 
> > This idea of identify cores by the compatible looks workable.
> > I'll update this series at next version.
> > Thanks!
> > 
> > > 
> > > > +
> > > >     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
> > > > -   scp->sram_base = devm_ioremap_resource(dev, res);
> > > > +   if (core_id == SCP_CORE_1)
> > > > +           scp->sram_base = devm_ioremap(dev, res->start,
> > 
> > resource_size(res));
> > > > +   else
> > > > +           scp->sram_base = devm_ioremap_resource(dev, res);
> > > > +
> > > 
> > > This looks very broken...  For this to work you would need to have two DT
> > > entries with the "mediatek,mt8195-scp-dual" compatible properly, one with
> > > "mediatek,scp-core = <&scp_dual1 0>;" and another one with
> > 
> > "mediatek,scp-core = <&scp_dual0 1>;".
> > > 
> > > Which is also very broken...  Here you have a binding whose first
> > 
> > argument is a
> > > reference to the core sibling while the second argument is a
> > 
> > characteristic of
> > > the current core, which is highly confusing.
> > > 
> > > I suggest what when you see the compatible binding
> > 
> > "mediatek,mt8195-scp", a
> > > single core is initialized.  If you see "mediatek,mt8195-scp-dual", both
> > 
> > cores
> > > are initialized as part of the _same_ probe.
> > > 
> > > If the above analysis is not correct it means I misinterpreted your
> > > work and if so, a serious amount of comments is needed _and_ a very
> > 
> > detailed
> > > example in "mtk,scp.yaml" that leaves no room for interpretation.
> > > 
> > > I will stop reviewing this patchset until you have clarified how this
> > 
> > works.
> > > 
> > > Thanks,
> > > Mathieu
> > 
> > There's one problem of initializng the CORE1 using the same probe flow.
> > The register space of CORE0 and CORE1 are overlapped in the device node.
> > Both cores need to use the 'cfg' registers defined in scp yaml.
> > The devm_ioremap_resource catches address overlapping and returns error
> > when
> > probing CORE1 driver.
> > 
> 
> That is exactly why I suggest to initialise both cores within the same
> probe() function.
> 

Hi Mathieu,

I'm thinking about how to initialise in the same probe() function.
I'm wondering if this implies that using one scp driver to initialize 2 cores?
If it is, I assume the dts descriptions for both cores should be contained in one node.

When there's one node for both cores, it looks like that there is a problem of 
using dma_allocate_coherent(). Each core has its own reserved memory region. 
When there's only one device for both cores, it's not able to identify the memory region 
by the device parameter of dma_allocate_coherent().

Is it acceptable to consider manually allocating core 1 device in the probe() when probing core 0?


Best regards,
TingHan









_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP

[Mathieu Poirier]

On Fri, 16 Sept 2022 at 06:00, TingHan Shen <tinghan.shen@mediatek.com> wrote:
>
> On Thu, 2022-09-08 at 14:58 -0600, Mathieu Poirier wrote:
> > On Thu, 8 Sept 2022 at 05:21, Tinghan Shen <tinghan.shen@mediatek.com>
> > wrote:
> >
> > > Hi Mathieu,
> > >
> > > > > The mtk_scp.c driver only supports the single core SCP and the
> > > > > 1st core of a dual-core SCP. This patch extends it for the 2nd core.
> > > > >
> > > > > MT8195 SCP is a dual-core MCU. Both cores are housed in the same
> > >
> > > subsys.
> > > >
> > > > s/subsys/subsystem
> > > >
> > > > > They have the same viewpoint of registers and memory.
> > > > >
> > > > > Core 1 of the SCP features its own set of core configuration registers,
> > > > > interrupt controller, timers, and DMAs. The rest of the peripherals
> > > > > in this subsystem are shared by core 0 and core 1.
> > > > >
> > > > > As for memory, core 1 has its own cache memory. the SCP SRAM is shared
> > > >
> > > > /the/The
> > > >
> > > > > by core 0 and core 1.
> > > > >
> > > > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > > > > ---
> > > > >  drivers/remoteproc/mtk_scp.c | 22 ++++++++++++++++++++--
> > > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/remoteproc/mtk_scp.c
> > >
> > > b/drivers/remoteproc/mtk_scp.c
> > > > > index 3510c6d0bbc8..91b4aefde4ac 100644
> > > > > --- a/drivers/remoteproc/mtk_scp.c
> > > > > +++ b/drivers/remoteproc/mtk_scp.c
> > > > > @@ -23,6 +23,10 @@
> > > > >  #define MAX_CODE_SIZE 0x500000
> > > > >  #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
> > > > >
> > > > > +#define SCP_CORE_0 0
> > > > > +#define SCP_CORE_1 1
> > > > > +#define SCP_CORE_SINGLE 0xF
> > > > > +
> > > > >  /**
> > > > >   * scp_get() - get a reference to SCP.
> > > > >   *
> > > > > @@ -836,6 +840,7 @@ static int scp_probe(struct platform_device *pdev)
> > > > >     struct resource *res;
> > > > >     const char *fw_name = "scp.img";
> > > > >     int ret, i;
> > > > > +   u32 core_id = SCP_CORE_SINGLE;
> > > > >
> > > > >     ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> > > > >     if (ret < 0 && ret != -EINVAL)
> > > > > @@ -851,8 +856,16 @@ static int scp_probe(struct platform_device *pdev)
> > > > >     scp->data = of_device_get_match_data(dev);
> > > > >     platform_set_drvdata(pdev, scp);
> > > > >
> > > > > +   ret = of_property_read_u32_index(dev->of_node,
> > >
> > > "mediatek,scp-core", 1, &core_id);
> > > > > +   if (ret == 0)
> > > > > +           dev_info(dev, "Boot SCP dual core %u\n", core_id);
> > > >
> > > > Why is the DT property "mediatek,scp-core" needed at all?  Since the
> > >
> > > compatible
> > > > "mediatek,mt8195-scp-dual" has already been defined previously in this
> > >
> > > patchset,
> > > > initialising the second core, if present, is a matter of looking at the
> > > > compatile string.
> > >
> > > This idea of identify cores by the compatible looks workable.
> > > I'll update this series at next version.
> > > Thanks!
> > >
> > > >
> > > > > +
> > > > >     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
> > > > > -   scp->sram_base = devm_ioremap_resource(dev, res);
> > > > > +   if (core_id == SCP_CORE_1)
> > > > > +           scp->sram_base = devm_ioremap(dev, res->start,
> > >
> > > resource_size(res));
> > > > > +   else
> > > > > +           scp->sram_base = devm_ioremap_resource(dev, res);
> > > > > +
> > > >
> > > > This looks very broken...  For this to work you would need to have two DT
> > > > entries with the "mediatek,mt8195-scp-dual" compatible properly, one with
> > > > "mediatek,scp-core = <&scp_dual1 0>;" and another one with
> > >
> > > "mediatek,scp-core = <&scp_dual0 1>;".
> > > >
> > > > Which is also very broken...  Here you have a binding whose first
> > >
> > > argument is a
> > > > reference to the core sibling while the second argument is a
> > >
> > > characteristic of
> > > > the current core, which is highly confusing.
> > > >
> > > > I suggest what when you see the compatible binding
> > >
> > > "mediatek,mt8195-scp", a
> > > > single core is initialized.  If you see "mediatek,mt8195-scp-dual", both
> > >
> > > cores
> > > > are initialized as part of the _same_ probe.
> > > >
> > > > If the above analysis is not correct it means I misinterpreted your
> > > > work and if so, a serious amount of comments is needed _and_ a very
> > >
> > > detailed
> > > > example in "mtk,scp.yaml" that leaves no room for interpretation.
> > > >
> > > > I will stop reviewing this patchset until you have clarified how this
> > >
> > > works.
> > > >
> > > > Thanks,
> > > > Mathieu
> > >
> > > There's one problem of initializng the CORE1 using the same probe flow.
> > > The register space of CORE0 and CORE1 are overlapped in the device node.
> > > Both cores need to use the 'cfg' registers defined in scp yaml.
> > > The devm_ioremap_resource catches address overlapping and returns error
> > > when
> > > probing CORE1 driver.
> > >
> >
> > That is exactly why I suggest to initialise both cores within the same
> > probe() function.
> >
>
> Hi Mathieu,
>
> I'm thinking about how to initialise in the same probe() function.
> I'm wondering if this implies that using one scp driver to initialize 2 cores?
> If it is, I assume the dts descriptions for both cores should be contained in one node.
>
> When there's one node for both cores, it looks like that there is a problem of
> using dma_allocate_coherent(). Each core has its own reserved memory region.
> When there's only one device for both cores, it's not able to identify the memory region
> by the device parameter of dma_allocate_coherent().
>
> Is it acceptable to consider manually allocating core 1 device in the probe() when probing core 0?

Look at what Suman did for TI's K3 R5[1] and DSP[2] platforms.
Reviewing the bindings for both platforms will also give you a good
idea of how things work.

[1]. https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_r5_remoteproc.c#L1683
[2]. https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_dsp_remoteproc.c#L673

>
>
> Best regards,
> TingHan
>
>
>
>
>
>
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP

[TingHan Shen]

On Fri, 2022-09-16 at 11:15 -0600, Mathieu Poirier wrote:
> On Fri, 16 Sept 2022 at 06:00, TingHan Shen <tinghan.shen@mediatek.com> wrote:
> > 
> > On Thu, 2022-09-08 at 14:58 -0600, Mathieu Poirier wrote:
> > > On Thu, 8 Sept 2022 at 05:21, Tinghan Shen <tinghan.shen@mediatek.com>
> > > wrote:
> > > 
> > > > Hi Mathieu,
> > > > 
> > > > > > The mtk_scp.c driver only supports the single core SCP and the
> > > > > > 1st core of a dual-core SCP. This patch extends it for the 2nd core.
> > > > > > 
> > > > > > MT8195 SCP is a dual-core MCU. Both cores are housed in the same
> > > > 
> > > > subsys.
> > > > > 
> > > > > s/subsys/subsystem
> > > > > 
> > > > > > They have the same viewpoint of registers and memory.
> > > > > > 
> > > > > > Core 1 of the SCP features its own set of core configuration registers,
> > > > > > interrupt controller, timers, and DMAs. The rest of the peripherals
> > > > > > in this subsystem are shared by core 0 and core 1.
> > > > > > 
> > > > > > As for memory, core 1 has its own cache memory. the SCP SRAM is shared
> > > > > 
> > > > > /the/The
> > > > > 
> > > > > > by core 0 and core 1.
> > > > > > 
> > > > > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > > > > > ---
> > > > > >  drivers/remoteproc/mtk_scp.c | 22 ++++++++++++++++++++--
> > > > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/remoteproc/mtk_scp.c
> > > > 
> > > > b/drivers/remoteproc/mtk_scp.c
> > > > > > index 3510c6d0bbc8..91b4aefde4ac 100644
> > > > > > --- a/drivers/remoteproc/mtk_scp.c
> > > > > > +++ b/drivers/remoteproc/mtk_scp.c
> > > > > > @@ -23,6 +23,10 @@
> > > > > >  #define MAX_CODE_SIZE 0x500000
> > > > > >  #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
> > > > > > 
> > > > > > +#define SCP_CORE_0 0
> > > > > > +#define SCP_CORE_1 1
> > > > > > +#define SCP_CORE_SINGLE 0xF
> > > > > > +
> > > > > >  /**
> > > > > >   * scp_get() - get a reference to SCP.
> > > > > >   *
> > > > > > @@ -836,6 +840,7 @@ static int scp_probe(struct platform_device *pdev)
> > > > > >     struct resource *res;
> > > > > >     const char *fw_name = "scp.img";
> > > > > >     int ret, i;
> > > > > > +   u32 core_id = SCP_CORE_SINGLE;
> > > > > > 
> > > > > >     ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> > > > > >     if (ret < 0 && ret != -EINVAL)
> > > > > > @@ -851,8 +856,16 @@ static int scp_probe(struct platform_device *pdev)
> > > > > >     scp->data = of_device_get_match_data(dev);
> > > > > >     platform_set_drvdata(pdev, scp);
> > > > > > 
> > > > > > +   ret = of_property_read_u32_index(dev->of_node,
> > > > 
> > > > "mediatek,scp-core", 1, &core_id);
> > > > > > +   if (ret == 0)
> > > > > > +           dev_info(dev, "Boot SCP dual core %u\n", core_id);
> > > > > 
> > > > > Why is the DT property "mediatek,scp-core" needed at all?  Since the
> > > > 
> > > > compatible
> > > > > "mediatek,mt8195-scp-dual" has already been defined previously in this
> > > > 
> > > > patchset,
> > > > > initialising the second core, if present, is a matter of looking at the
> > > > > compatile string.
> > > > 
> > > > This idea of identify cores by the compatible looks workable.
> > > > I'll update this series at next version.
> > > > Thanks!
> > > > 
> > > > > 
> > > > > > +
> > > > > >     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
> > > > > > -   scp->sram_base = devm_ioremap_resource(dev, res);
> > > > > > +   if (core_id == SCP_CORE_1)
> > > > > > +           scp->sram_base = devm_ioremap(dev, res->start,
> > > > 
> > > > resource_size(res));
> > > > > > +   else
> > > > > > +           scp->sram_base = devm_ioremap_resource(dev, res);
> > > > > > +
> > > > > 
> > > > > This looks very broken...  For this to work you would need to have two DT
> > > > > entries with the "mediatek,mt8195-scp-dual" compatible properly, one with
> > > > > "mediatek,scp-core = <&scp_dual1 0>;" and another one with
> > > > 
> > > > "mediatek,scp-core = <&scp_dual0 1>;".
> > > > > 
> > > > > Which is also very broken...  Here you have a binding whose first
> > > > 
> > > > argument is a
> > > > > reference to the core sibling while the second argument is a
> > > > 
> > > > characteristic of
> > > > > the current core, which is highly confusing.
> > > > > 
> > > > > I suggest what when you see the compatible binding
> > > > 
> > > > "mediatek,mt8195-scp", a
> > > > > single core is initialized.  If you see "mediatek,mt8195-scp-dual", both
> > > > 
> > > > cores
> > > > > are initialized as part of the _same_ probe.
> > > > > 
> > > > > If the above analysis is not correct it means I misinterpreted your
> > > > > work and if so, a serious amount of comments is needed _and_ a very
> > > > 
> > > > detailed
> > > > > example in "mtk,scp.yaml" that leaves no room for interpretation.
> > > > > 
> > > > > I will stop reviewing this patchset until you have clarified how this
> > > > 
> > > > works.
> > > > > 
> > > > > Thanks,
> > > > > Mathieu
> > > > 
> > > > There's one problem of initializng the CORE1 using the same probe flow.
> > > > The register space of CORE0 and CORE1 are overlapped in the device node.
> > > > Both cores need to use the 'cfg' registers defined in scp yaml.
> > > > The devm_ioremap_resource catches address overlapping and returns error
> > > > when
> > > > probing CORE1 driver.
> > > > 
> > > 
> > > That is exactly why I suggest to initialise both cores within the same
> > > probe() function.
> > > 
> > 
> > Hi Mathieu,
> > 
> > I'm thinking about how to initialise in the same probe() function.
> > I'm wondering if this implies that using one scp driver to initialize 2 cores?
> > If it is, I assume the dts descriptions for both cores should be contained in one node.
> > 
> > When there's one node for both cores, it looks like that there is a problem of
> > using dma_allocate_coherent(). Each core has its own reserved memory region.
> > When there's only one device for both cores, it's not able to identify the memory region
> > by the device parameter of dma_allocate_coherent().
> > 
> > Is it acceptable to consider manually allocating core 1 device in the probe() when probing core 0?
> 
> Look at what Suman did for TI's K3 R5[1] and DSP[2] platforms.
> Reviewing the bindings for both platforms will also give you a good
> idea of how things work.
> 
> [1]. https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_r5_remoteproc.c*L1683__;Iw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX30z0XV4vo3vCb9Wy-w5ixOTmzbv1akubM$ 
> [2]. https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_dsp_remoteproc.c*L673__;Iw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX30z0XV4vo3vCb9Wy-w5ixOTmzbfE2dtBg$ 
> 

Hi Mathieu,

My plan is changing the dts as following,

scp core 0 {
	// Keep current properties untouched.
	compatible = "mediatek,mt8195-scp";

	// core 0 properties...

	// Add a new property for multi-core scp.
	// if not present, it's single core.
	// if present and core id = 0, it's the main core, otherwise the sub cores.
	mediatek,scp-core = <0>;

	// add sub cores as sub node.
	// sub nodes can find parent by OF API.
	scp core 1 {
		// use the same compatile name as core 0.
		compatible = "mediatek,mt8195-scp";

		// assign id > 0 to sub cores.
		mediatek,scp-core = <1>;

		// core 1 properties...
	};
};


The driver probe/remove behavior will be modified as below,

scp probe() {
	// common init...

	// check core id to have different memory mapping flow
	if (core id == 0)
		// mapping cfg, sram and others
	else
		// mapping sram
		// reuse the cfg paddr/vaddr from core 0
	
	// common init...

	if (core id == 0) {
		ret = of_platform_populate(...)

		// boot core 0 and sub cores
		rproc_add();
	} else {
		// add sub core as sub device to main core
		rproc_add_subdev()

		rproc->auto_boot = false;
		rpoc_add();
	}
}

scp_remove() {

	if (core id == 0)
		of_platform_depopulate()
	else
		rproc_remove_subdev()	
	
	// remove core
}



Best regards,
TingHan





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP

[Mathieu Poirier]

On Mon, 19 Sept 2022 at 03:46, TingHan Shen <tinghan.shen@mediatek.com> wrote:
>
> On Fri, 2022-09-16 at 11:15 -0600, Mathieu Poirier wrote:
> > On Fri, 16 Sept 2022 at 06:00, TingHan Shen <tinghan.shen@mediatek.com> wrote:
> > >
> > > On Thu, 2022-09-08 at 14:58 -0600, Mathieu Poirier wrote:
> > > > On Thu, 8 Sept 2022 at 05:21, Tinghan Shen <tinghan.shen@mediatek.com>
> > > > wrote:
> > > >
> > > > > Hi Mathieu,
> > > > >
> > > > > > > The mtk_scp.c driver only supports the single core SCP and the
> > > > > > > 1st core of a dual-core SCP. This patch extends it for the 2nd core.
> > > > > > >
> > > > > > > MT8195 SCP is a dual-core MCU. Both cores are housed in the same
> > > > >
> > > > > subsys.
> > > > > >
> > > > > > s/subsys/subsystem
> > > > > >
> > > > > > > They have the same viewpoint of registers and memory.
> > > > > > >
> > > > > > > Core 1 of the SCP features its own set of core configuration registers,
> > > > > > > interrupt controller, timers, and DMAs. The rest of the peripherals
> > > > > > > in this subsystem are shared by core 0 and core 1.
> > > > > > >
> > > > > > > As for memory, core 1 has its own cache memory. the SCP SRAM is shared
> > > > > >
> > > > > > /the/The
> > > > > >
> > > > > > > by core 0 and core 1.
> > > > > > >
> > > > > > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > > > > > > ---
> > > > > > >  drivers/remoteproc/mtk_scp.c | 22 ++++++++++++++++++++--
> > > > > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/remoteproc/mtk_scp.c
> > > > >
> > > > > b/drivers/remoteproc/mtk_scp.c
> > > > > > > index 3510c6d0bbc8..91b4aefde4ac 100644
> > > > > > > --- a/drivers/remoteproc/mtk_scp.c
> > > > > > > +++ b/drivers/remoteproc/mtk_scp.c
> > > > > > > @@ -23,6 +23,10 @@
> > > > > > >  #define MAX_CODE_SIZE 0x500000
> > > > > > >  #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
> > > > > > >
> > > > > > > +#define SCP_CORE_0 0
> > > > > > > +#define SCP_CORE_1 1
> > > > > > > +#define SCP_CORE_SINGLE 0xF
> > > > > > > +
> > > > > > >  /**
> > > > > > >   * scp_get() - get a reference to SCP.
> > > > > > >   *
> > > > > > > @@ -836,6 +840,7 @@ static int scp_probe(struct platform_device *pdev)
> > > > > > >     struct resource *res;
> > > > > > >     const char *fw_name = "scp.img";
> > > > > > >     int ret, i;
> > > > > > > +   u32 core_id = SCP_CORE_SINGLE;
> > > > > > >
> > > > > > >     ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> > > > > > >     if (ret < 0 && ret != -EINVAL)
> > > > > > > @@ -851,8 +856,16 @@ static int scp_probe(struct platform_device *pdev)
> > > > > > >     scp->data = of_device_get_match_data(dev);
> > > > > > >     platform_set_drvdata(pdev, scp);
> > > > > > >
> > > > > > > +   ret = of_property_read_u32_index(dev->of_node,
> > > > >
> > > > > "mediatek,scp-core", 1, &core_id);
> > > > > > > +   if (ret == 0)
> > > > > > > +           dev_info(dev, "Boot SCP dual core %u\n", core_id);
> > > > > >
> > > > > > Why is the DT property "mediatek,scp-core" needed at all?  Since the
> > > > >
> > > > > compatible
> > > > > > "mediatek,mt8195-scp-dual" has already been defined previously in this
> > > > >
> > > > > patchset,
> > > > > > initialising the second core, if present, is a matter of looking at the
> > > > > > compatile string.
> > > > >
> > > > > This idea of identify cores by the compatible looks workable.
> > > > > I'll update this series at next version.
> > > > > Thanks!
> > > > >
> > > > > >
> > > > > > > +
> > > > > > >     res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
> > > > > > > -   scp->sram_base = devm_ioremap_resource(dev, res);
> > > > > > > +   if (core_id == SCP_CORE_1)
> > > > > > > +           scp->sram_base = devm_ioremap(dev, res->start,
> > > > >
> > > > > resource_size(res));
> > > > > > > +   else
> > > > > > > +           scp->sram_base = devm_ioremap_resource(dev, res);
> > > > > > > +
> > > > > >
> > > > > > This looks very broken...  For this to work you would need to have two DT
> > > > > > entries with the "mediatek,mt8195-scp-dual" compatible properly, one with
> > > > > > "mediatek,scp-core = <&scp_dual1 0>;" and another one with
> > > > >
> > > > > "mediatek,scp-core = <&scp_dual0 1>;".
> > > > > >
> > > > > > Which is also very broken...  Here you have a binding whose first
> > > > >
> > > > > argument is a
> > > > > > reference to the core sibling while the second argument is a
> > > > >
> > > > > characteristic of
> > > > > > the current core, which is highly confusing.
> > > > > >
> > > > > > I suggest what when you see the compatible binding
> > > > >
> > > > > "mediatek,mt8195-scp", a
> > > > > > single core is initialized.  If you see "mediatek,mt8195-scp-dual", both
> > > > >
> > > > > cores
> > > > > > are initialized as part of the _same_ probe.
> > > > > >
> > > > > > If the above analysis is not correct it means I misinterpreted your
> > > > > > work and if so, a serious amount of comments is needed _and_ a very
> > > > >
> > > > > detailed
> > > > > > example in "mtk,scp.yaml" that leaves no room for interpretation.
> > > > > >
> > > > > > I will stop reviewing this patchset until you have clarified how this
> > > > >
> > > > > works.
> > > > > >
> > > > > > Thanks,
> > > > > > Mathieu
> > > > >
> > > > > There's one problem of initializng the CORE1 using the same probe flow.
> > > > > The register space of CORE0 and CORE1 are overlapped in the device node.
> > > > > Both cores need to use the 'cfg' registers defined in scp yaml.
> > > > > The devm_ioremap_resource catches address overlapping and returns error
> > > > > when
> > > > > probing CORE1 driver.
> > > > >
> > > >
> > > > That is exactly why I suggest to initialise both cores within the same
> > > > probe() function.
> > > >
> > >
> > > Hi Mathieu,
> > >
> > > I'm thinking about how to initialise in the same probe() function.
> > > I'm wondering if this implies that using one scp driver to initialize 2 cores?
> > > If it is, I assume the dts descriptions for both cores should be contained in one node.
> > >
> > > When there's one node for both cores, it looks like that there is a problem of
> > > using dma_allocate_coherent(). Each core has its own reserved memory region.
> > > When there's only one device for both cores, it's not able to identify the memory region
> > > by the device parameter of dma_allocate_coherent().
> > >
> > > Is it acceptable to consider manually allocating core 1 device in the probe() when probing core 0?
> >
> > Look at what Suman did for TI's K3 R5[1] and DSP[2] platforms.
> > Reviewing the bindings for both platforms will also give you a good
> > idea of how things work.
> >
> > [1]. https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_r5_remoteproc.c*L1683__;Iw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX30z0XV4vo3vCb9Wy-w5ixOTmzbv1akubM$
> > [2]. https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/remoteproc/ti_k3_dsp_remoteproc.c*L673__;Iw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX30z0XV4vo3vCb9Wy-w5ixOTmzbfE2dtBg$
> >
>
> Hi Mathieu,
>
> My plan is changing the dts as following,
>
> scp core 0 {
>         // Keep current properties untouched.
>         compatible = "mediatek,mt8195-scp";
>
>         // core 0 properties...
>
>         // Add a new property for multi-core scp.
>         // if not present, it's single core.
>         // if present and core id = 0, it's the main core, otherwise the sub cores.
>         mediatek,scp-core = <0>;
>
>         // add sub cores as sub node.
>         // sub nodes can find parent by OF API.
>         scp core 1 {
>                 // use the same compatile name as core 0.
>                 compatible = "mediatek,mt8195-scp";
>
>                 // assign id > 0 to sub cores.
>                 mediatek,scp-core = <1>;
>
>                 // core 1 properties...
>         };
> };
>
>
> The driver probe/remove behavior will be modified as below,
>
> scp probe() {
>         // common init...
>
>         // check core id to have different memory mapping flow
>         if (core id == 0)
>                 // mapping cfg, sram and others
>         else
>                 // mapping sram
>                 // reuse the cfg paddr/vaddr from core 0
>
>         // common init...
>
>         if (core id == 0) {
>                 ret = of_platform_populate(...)
>
>                 // boot core 0 and sub cores
>                 rproc_add();
>         } else {
>                 // add sub core as sub device to main core
>                 rproc_add_subdev()
>
>                 rproc->auto_boot = false;
>                 rpoc_add();
>         }
> }
>
> scp_remove() {
>
>         if (core id == 0)
>                 of_platform_depopulate()
>         else
>                 rproc_remove_subdev()
>
>         // remove core
> }
>

Unfortunately I do not have the bandwidth to look at inlined or pseudo
code.  I will review this code with the next revision.

>
>
> Best regards,
> TingHan
>
>
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the 2nd core of dual-core SCP

[Peng Fan]

> Subject: Re: [PATCH v2 4/9] remoteproc: mediatek: Support probing for the
> 2nd core of dual-core SCP
> 
> On Fri, 2022-09-16 at 11:15 -0600, Mathieu Poirier wrote:
> > On Fri, 16 Sept 2022 at 06:00, TingHan Shen
> <tinghan.shen@mediatek.com> wrote:
> > >
> > > On Thu, 2022-09-08 at 14:58 -0600, Mathieu Poirier wrote:
> > > > On Thu, 8 Sept 2022 at 05:21, Tinghan Shen
> > > > <tinghan.shen@mediatek.com>
> > > > wrote:
> > > >
> > > > > Hi Mathieu,
> > > > >
> > > > > > > The mtk_scp.c driver only supports the single core SCP and
> > > > > > > the 1st core of a dual-core SCP. This patch extends it for the 2nd
> core.
> > > > > > >
> > > > > > > MT8195 SCP is a dual-core MCU. Both cores are housed in the
> > > > > > > same
> > > > >
> > > > > subsys.
> > > > > >
> > > > > > s/subsys/subsystem
> > > > > >
> > > > > > > They have the same viewpoint of registers and memory.
> > > > > > >
> > > > > > > Core 1 of the SCP features its own set of core configuration
> > > > > > > registers, interrupt controller, timers, and DMAs. The rest
> > > > > > > of the peripherals in this subsystem are shared by core 0 and
> core 1.
> > > > > > >
> > > > > > > As for memory, core 1 has its own cache memory. the SCP SRAM
> > > > > > > is shared
> > > > > >
> > > > > > /the/The
> > > > > >
> > > > > > > by core 0 and core 1.
> > > > > > >
> > > > > > > Signed-off-by: Tinghan Shen <tinghan.shen@mediatek.com>
> > > > > > > ---
> > > > > > >  drivers/remoteproc/mtk_scp.c | 22 ++++++++++++++++++++--
> > > > > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/remoteproc/mtk_scp.c
> > > > >
> > > > > b/drivers/remoteproc/mtk_scp.c
> > > > > > > index 3510c6d0bbc8..91b4aefde4ac 100644
> > > > > > > --- a/drivers/remoteproc/mtk_scp.c
> > > > > > > +++ b/drivers/remoteproc/mtk_scp.c
> > > > > > > @@ -23,6 +23,10 @@
> > > > > > >  #define MAX_CODE_SIZE 0x500000  #define
> > > > > > > SECTION_NAME_IPI_BUFFER ".ipi_buffer"
> > > > > > >
> > > > > > > +#define SCP_CORE_0 0
> > > > > > > +#define SCP_CORE_1 1
> > > > > > > +#define SCP_CORE_SINGLE 0xF
> > > > > > > +
> > > > > > >  /**
> > > > > > >   * scp_get() - get a reference to SCP.
> > > > > > >   *
> > > > > > > @@ -836,6 +840,7 @@ static int scp_probe(struct
> platform_device *pdev)
> > > > > > >     struct resource *res;
> > > > > > >     const char *fw_name = "scp.img";
> > > > > > >     int ret, i;
> > > > > > > +   u32 core_id = SCP_CORE_SINGLE;
> > > > > > >
> > > > > > >     ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> > > > > > >     if (ret < 0 && ret != -EINVAL) @@ -851,8 +856,16 @@
> > > > > > > static int scp_probe(struct platform_device *pdev)
> > > > > > >     scp->data = of_device_get_match_data(dev);
> > > > > > >     platform_set_drvdata(pdev, scp);
> > > > > > >
> > > > > > > +   ret = of_property_read_u32_index(dev->of_node,
> > > > >
> > > > > "mediatek,scp-core", 1, &core_id);
> > > > > > > +   if (ret == 0)
> > > > > > > +           dev_info(dev, "Boot SCP dual core %u\n",
> > > > > > > + core_id);
> > > > > >
> > > > > > Why is the DT property "mediatek,scp-core" needed at all?
> > > > > > Since the
> > > > >
> > > > > compatible
> > > > > > "mediatek,mt8195-scp-dual" has already been defined previously
> > > > > > in this
> > > > >
> > > > > patchset,
> > > > > > initialising the second core, if present, is a matter of
> > > > > > looking at the compatile string.
> > > > >
> > > > > This idea of identify cores by the compatible looks workable.
> > > > > I'll update this series at next version.
> > > > > Thanks!
> > > > >
> > > > > >
> > > > > > > +
> > > > > > >     res = platform_get_resource_byname(pdev,
> IORESOURCE_MEM, "sram");
> > > > > > > -   scp->sram_base = devm_ioremap_resource(dev, res);
> > > > > > > +   if (core_id == SCP_CORE_1)
> > > > > > > +           scp->sram_base = devm_ioremap(dev, res->start,
> > > > >
> > > > > resource_size(res));
> > > > > > > +   else
> > > > > > > +           scp->sram_base = devm_ioremap_resource(dev,
> > > > > > > + res);
> > > > > > > +
> > > > > >
> > > > > > This looks very broken...  For this to work you would need to
> > > > > > have two DT entries with the "mediatek,mt8195-scp-dual"
> > > > > > compatible properly, one with "mediatek,scp-core = <&scp_dual1
> > > > > > 0>;" and another one with
> > > > >
> > > > > "mediatek,scp-core = <&scp_dual0 1>;".
> > > > > >
> > > > > > Which is also very broken...  Here you have a binding whose
> > > > > > first
> > > > >
> > > > > argument is a
> > > > > > reference to the core sibling while the second argument is a
> > > > >
> > > > > characteristic of
> > > > > > the current core, which is highly confusing.
> > > > > >
> > > > > > I suggest what when you see the compatible binding
> > > > >
> > > > > "mediatek,mt8195-scp", a
> > > > > > single core is initialized.  If you see
> > > > > > "mediatek,mt8195-scp-dual", both
> > > > >
> > > > > cores
> > > > > > are initialized as part of the _same_ probe.
> > > > > >
> > > > > > If the above analysis is not correct it means I misinterpreted
> > > > > > your work and if so, a serious amount of comments is needed
> > > > > > _and_ a very
> > > > >
> > > > > detailed
> > > > > > example in "mtk,scp.yaml" that leaves no room for interpretation.
> > > > > >
> > > > > > I will stop reviewing this patchset until you have clarified
> > > > > > how this
> > > > >
> > > > > works.
> > > > > >
> > > > > > Thanks,
> > > > > > Mathieu
> > > > >
> > > > > There's one problem of initializng the CORE1 using the same probe
> flow.
> > > > > The register space of CORE0 and CORE1 are overlapped in the device
> node.
> > > > > Both cores need to use the 'cfg' registers defined in scp yaml.
> > > > > The devm_ioremap_resource catches address overlapping and
> > > > > returns error when probing CORE1 driver.
> > > > >
> > > >
> > > > That is exactly why I suggest to initialise both cores within the same
> > > > probe() function.
> > > >
> > >
> > > Hi Mathieu,
> > >
> > > I'm thinking about how to initialise in the same probe() function.
> > > I'm wondering if this implies that using one scp driver to initialize 2 cores?
> > > If it is, I assume the dts descriptions for both cores should be contained
> in one node.
> > >
> > > When there's one node for both cores, it looks like that there is a
> problem of
> > > using dma_allocate_coherent(). Each core has its own reserved memory
> region.
> > > When there's only one device for both cores, it's not able to identify the
> memory region
> > > by the device parameter of dma_allocate_coherent().
> > >
> > > Is it acceptable to consider manually allocating core 1 device in the
> probe() when probing core 0?
> >
> > Look at what Suman did for TI's K3 R5[1] and DSP[2] platforms.
> > Reviewing the bindings for both platforms will also give you a good
> > idea of how things work.
> >
> > [1].
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furlde
> fense.com%2Fv3%2F__https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.0-
> rc5%2Fsource%2Fdrivers%2Fremoteproc%2Fti_k3_r5_remoteproc.c*L1683_
> _%3BIw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX3
> 0z0XV4vo3vCb9Wy-
> w5ixOTmzbv1akubM%24&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7
> C2cf76b2f15544cf3d06308da9a23f00a%7C686ea1d3bc2b4c6fa92cd99c5c301
> 635%7C0%7C0%7C637991776400238974%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3000%7C%7C%7C&amp;sdata=dz0FlQQmKI4C67XCX%2BZ6%2Bin%2Btq
> 2DEWLb5YA%2FxGLOxHc%3D&amp;reserved=0
> > [2].
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furlde
> fense.com%2Fv3%2F__https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.0-
> rc5%2Fsource%2Fdrivers%2Fremoteproc%2Fti_k3_dsp_remoteproc.c*L673_
> _%3BIw!!CTRNKA9wMg0ARbw!zVcjdLSfKYGO5YQMNGqq339mle8u0VdULX3
> 0z0XV4vo3vCb9Wy-
> w5ixOTmzbfE2dtBg%24&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7C
> 2cf76b2f15544cf3d06308da9a23f00a%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%7C0%7C0%7C637991776400238974%7CUnknown%7CTWFpbGZsb3d8ey
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3000%7C%7C%7C&amp;sdata=Yn7MOo2uoDOVRW47O1yq8W3c%2BYg
> G5URr7RdLKsmpLrk%3D&amp;reserved=0
> >
> 
> Hi Mathieu,
> 
> My plan is changing the dts as following,
> 
> scp core 0 {
> 	// Keep current properties untouched.
> 	compatible = "mediatek,mt8195-scp";
> 
> 	// core 0 properties...
> 
> 	// Add a new property for multi-core scp.
> 	// if not present, it's single core.
> 	// if present and core id = 0, it's the main core, otherwise the sub
> cores.
> 	mediatek,scp-core = <0>;
> 
> 	// add sub cores as sub node.
> 	// sub nodes can find parent by OF API.
> 	scp core 1 {
> 		// use the same compatile name as core 0.
> 		compatible = "mediatek,mt8195-scp";
> 
> 		// assign id > 0 to sub cores.
> 		mediatek,scp-core = <1>;
> 
> 		// core 1 properties...
> 	};
> };

Not know the HW arch, but this looks a bit weird, scp core 1 is
a sub node of scp core 0.

> 
> 
> The driver probe/remove behavior will be modified as below,
> 
> scp probe() {
> 	// common init...
> 
> 	// check core id to have different memory mapping flow
> 	if (core id == 0)
> 		// mapping cfg, sram and others
> 	else
> 		// mapping sram
> 		// reuse the cfg paddr/vaddr from core 0

Oh, I understand why you need ioremap_resource, so core0/1 share
same register address space?

Regards,
Peng.
> 
> 	// common init...
> 
> 	if (core id == 0) {
> 		ret = of_platform_populate(...)
> 
> 		// boot core 0 and sub cores
> 		rproc_add();
> 	} else {
> 		// add sub core as sub device to main core
> 		rproc_add_subdev()
> 
> 		rproc->auto_boot = false;
> 		rpoc_add();
> 	}
> }
> 
> scp_remove() {
> 
> 	if (core id == 0)
> 		of_platform_depopulate()
> 	else
> 		rproc_remove_subdev()
> 
> 	// remove core
> }
> 
> 
> 
> Best regards,
> TingHan
> 
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel