[PATCH 0/2] Support MT8188 SCP core 1

[PATCH 0/2] Support MT8188 SCP core 1

[olivia.wen]

Add below patches to support MT8188 SCP core 1
[PATCH 1/2] dt-bindings: remoteproc: mediatek: 
               Support MT8188 dual-core SCP
[PATCH 2/2] remoteproc: mediatek: Support MT8188 SCP core 1

olivia.wen (2):
  dt-bindings: remoteproc: mediatek: Support MT8188 dual-core SCP
  remoteproc: mediatek: Support MT8188 SCP core 1

 .../devicetree/bindings/remoteproc/mtk,scp.yaml    |  3 +-
 drivers/remoteproc/mtk_common.h                    |  5 +-
 drivers/remoteproc/mtk_scp.c                       | 62 ++++++++++++++++++----
 drivers/remoteproc/mtk_scp_ipi.c                   |  9 +++-
 include/linux/remoteproc/mtk_scp.h                 |  1 +
 5 files changed, 64 insertions(+), 16 deletions(-)

-- 
2.6.4

[PATCH 1/2] dt-bindings: remoteproc: mediatek: Support MT8188 dual-core SCP

[olivia.wen]

Under different applications, the MT8188 SCP can be used as single-core
or dual-core.

Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
---
 Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
index 507f98f..7e7b567 100644
--- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
@@ -22,7 +22,7 @@ properties:
       - mediatek,mt8192-scp
       - mediatek,mt8195-scp
       - mediatek,mt8195-scp-dual
-
+      - mediatek,mt8188-scp-dual
   reg:
     description:
       Should contain the address ranges for memory regions SRAM, CFG, and,
@@ -195,6 +195,7 @@ allOf:
         compatible:
           enum:
             - mediatek,mt8195-scp-dual
+            - mediatek,mt8188-scp-dual
     then:
       properties:
         reg:
-- 
2.6.4

[PATCH 2/2] remoteproc: mediatek: Support MT8188 SCP core 1

[olivia.wen]

To Support MT8188 SCP core 1 for ISP driver.
The SCP on different chips will require different code sizes
 and IPI buffer sizes based on varying requirements.

Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
---
 drivers/remoteproc/mtk_common.h    |  5 +--
 drivers/remoteproc/mtk_scp.c       | 62 +++++++++++++++++++++++++++++++-------
 drivers/remoteproc/mtk_scp_ipi.c   |  9 ++++--
 include/linux/remoteproc/mtk_scp.h |  1 +
 4 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
index 6d7736a..8f37f65 100644
--- a/drivers/remoteproc/mtk_common.h
+++ b/drivers/remoteproc/mtk_common.h
@@ -78,7 +78,6 @@
 #define MT8195_L2TCM_OFFSET			0x850d0
 
 #define SCP_FW_VER_LEN			32
-#define SCP_SHARE_BUFFER_SIZE		288
 
 struct scp_run {
 	u32 signaled;
@@ -110,6 +109,8 @@ struct mtk_scp_of_data {
 	u32 host_to_scp_int_bit;
 
 	size_t ipi_buf_offset;
+	u32 ipi_buffer_size;
+	u32 max_code_size;
 };
 
 struct mtk_scp_of_cluster {
@@ -162,7 +163,7 @@ struct mtk_scp {
 struct mtk_share_obj {
 	u32 id;
 	u32 len;
-	u8 share_buf[SCP_SHARE_BUFFER_SIZE];
+	u8 *share_buf;
 };
 
 void scp_memcpy_aligned(void __iomem *dst, const void *src, unsigned int len);
diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index 6751829..270718d 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -20,7 +20,6 @@
 #include "mtk_common.h"
 #include "remoteproc_internal.h"
 
-#define MAX_CODE_SIZE 0x500000
 #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
 
 /**
@@ -94,14 +93,14 @@ static void scp_ipi_handler(struct mtk_scp *scp)
 {
 	struct mtk_share_obj __iomem *rcv_obj = scp->recv_buf;
 	struct scp_ipi_desc *ipi_desc = scp->ipi_desc;
-	u8 tmp_data[SCP_SHARE_BUFFER_SIZE];
+	u8 *tmp_data;
 	scp_ipi_handler_t handler;
 	u32 id = readl(&rcv_obj->id);
 	u32 len = readl(&rcv_obj->len);
 
-	if (len > SCP_SHARE_BUFFER_SIZE) {
+	if (len > scp->data->ipi_buffer_size) {
 		dev_err(scp->dev, "ipi message too long (len %d, max %d)", len,
-			SCP_SHARE_BUFFER_SIZE);
+			scp->data->ipi_buffer_size);
 		return;
 	}
 	if (id >= SCP_IPI_MAX) {
@@ -109,6 +108,10 @@ static void scp_ipi_handler(struct mtk_scp *scp)
 		return;
 	}
 
+	tmp_data = kzalloc(len, GFP_KERNEL);
+	if (!tmp_data)
+		return;
+
 	scp_ipi_lock(scp, id);
 	handler = ipi_desc[id].handler;
 	if (!handler) {
@@ -123,6 +126,7 @@ static void scp_ipi_handler(struct mtk_scp *scp)
 
 	scp->ipi_id_ack[id] = true;
 	wake_up(&scp->ack_wq);
+	kfree(tmp_data);
 }
 
 static int scp_elf_read_ipi_buf_addr(struct mtk_scp *scp,
@@ -133,6 +137,7 @@ static int scp_ipi_init(struct mtk_scp *scp, const struct firmware *fw)
 {
 	int ret;
 	size_t buf_sz, offset;
+	size_t share_buf_offset;
 
 	/* read the ipi buf addr from FW itself first */
 	ret = scp_elf_read_ipi_buf_addr(scp, fw, &offset);
@@ -154,10 +159,12 @@ static int scp_ipi_init(struct mtk_scp *scp, const struct firmware *fw)
 
 	scp->recv_buf = (struct mtk_share_obj __iomem *)
 			(scp->sram_base + offset);
+	share_buf_offset = sizeof(scp->recv_buf->id)
+		+ sizeof(scp->recv_buf->len) + scp->data->ipi_buffer_size;
 	scp->send_buf = (struct mtk_share_obj __iomem *)
-			(scp->sram_base + offset + sizeof(*scp->recv_buf));
-	memset_io(scp->recv_buf, 0, sizeof(*scp->recv_buf));
-	memset_io(scp->send_buf, 0, sizeof(*scp->send_buf));
+			(scp->sram_base + offset + share_buf_offset);
+	memset_io(scp->recv_buf, 0, share_buf_offset);
+	memset_io(scp->send_buf, 0, share_buf_offset);
 
 	return 0;
 }
@@ -891,7 +898,7 @@ static int scp_map_memory_region(struct mtk_scp *scp)
 	}
 
 	/* Reserved SCP code size */
-	scp->dram_size = MAX_CODE_SIZE;
+	scp->dram_size = scp->data->max_code_size;
 	scp->cpu_addr = dma_alloc_coherent(scp->dev, scp->dram_size,
 					   &scp->dma_addr, GFP_KERNEL);
 	if (!scp->cpu_addr)
@@ -1247,6 +1254,8 @@ static const struct mtk_scp_of_data mt8183_of_data = {
 	.host_to_scp_reg = MT8183_HOST_TO_SCP,
 	.host_to_scp_int_bit = MT8183_HOST_IPC_INT_BIT,
 	.ipi_buf_offset = 0x7bdb0,
+	.max_code_size = 0x500000,
+	.ipi_buffer_size = 288,
 };
 
 static const struct mtk_scp_of_data mt8186_of_data = {
@@ -1260,18 +1269,22 @@ static const struct mtk_scp_of_data mt8186_of_data = {
 	.host_to_scp_reg = MT8183_HOST_TO_SCP,
 	.host_to_scp_int_bit = MT8183_HOST_IPC_INT_BIT,
 	.ipi_buf_offset = 0x3bdb0,
+	.max_code_size = 0x500000,
+	.ipi_buffer_size = 288,
 };
 
 static const struct mtk_scp_of_data mt8188_of_data = {
 	.scp_clk_get = mt8195_scp_clk_get,
-	.scp_before_load = mt8192_scp_before_load,
-	.scp_irq_handler = mt8192_scp_irq_handler,
+	.scp_before_load = mt8195_scp_before_load,
+	.scp_irq_handler = mt8195_scp_irq_handler,
 	.scp_reset_assert = mt8192_scp_reset_assert,
 	.scp_reset_deassert = mt8192_scp_reset_deassert,
-	.scp_stop = mt8192_scp_stop,
+	.scp_stop = mt8195_scp_stop,
 	.scp_da_to_va = mt8192_scp_da_to_va,
 	.host_to_scp_reg = MT8192_GIPC_IN_SET,
 	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
+	.max_code_size = 0x500000,
+	.ipi_buffer_size = 600,
 };
 
 static const struct mtk_scp_of_data mt8192_of_data = {
@@ -1284,6 +1297,8 @@ static const struct mtk_scp_of_data mt8192_of_data = {
 	.scp_da_to_va = mt8192_scp_da_to_va,
 	.host_to_scp_reg = MT8192_GIPC_IN_SET,
 	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
+	.max_code_size = 0x500000,
+	.ipi_buffer_size = 288,
 };
 
 static const struct mtk_scp_of_data mt8195_of_data = {
@@ -1296,6 +1311,8 @@ static const struct mtk_scp_of_data mt8195_of_data = {
 	.scp_da_to_va = mt8192_scp_da_to_va,
 	.host_to_scp_reg = MT8192_GIPC_IN_SET,
 	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
+	.max_code_size = 0x500000,
+	.ipi_buffer_size = 288,
 };
 
 static const struct mtk_scp_of_data mt8195_of_data_c1 = {
@@ -1308,6 +1325,22 @@ static const struct mtk_scp_of_data mt8195_of_data_c1 = {
 	.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,
+	.max_code_size = 0x500000,
+	.ipi_buffer_size = 288,
+};
+
+static const struct mtk_scp_of_data mt8188_of_data_c1 = {
+	.scp_clk_get = mt8195_scp_clk_get,
+	.scp_before_load = mt8195_scp_c1_before_load,
+	.scp_irq_handler = mt8195_scp_c1_irq_handler,
+	.scp_reset_assert = mt8195_scp_c1_reset_assert,
+	.scp_reset_deassert = mt8195_scp_c1_reset_deassert,
+	.scp_stop = mt8195_scp_c1_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,
+	.max_code_size = 0xA00000,
+	.ipi_buffer_size = 600,
 };
 
 static const struct mtk_scp_of_data *mt8195_of_data_cores[] = {
@@ -1316,6 +1349,12 @@ static const struct mtk_scp_of_data *mt8195_of_data_cores[] = {
 	NULL
 };
 
+static const struct mtk_scp_of_data *mt8188_of_data_cores[] = {
+	&mt8188_of_data,
+	&mt8188_of_data_c1,
+	NULL
+};
+
 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 },
@@ -1323,6 +1362,7 @@ static const struct of_device_id mtk_scp_of_match[] = {
 	{ .compatible = "mediatek,mt8192-scp", .data = &mt8192_of_data },
 	{ .compatible = "mediatek,mt8195-scp", .data = &mt8195_of_data },
 	{ .compatible = "mediatek,mt8195-scp-dual", .data = &mt8195_of_data_cores },
+	{ .compatible = "mediatek,mt8188-scp-dual", .data = &mt8188_of_data_cores },
 	{},
 };
 MODULE_DEVICE_TABLE(of, mtk_scp_of_match);
diff --git a/drivers/remoteproc/mtk_scp_ipi.c b/drivers/remoteproc/mtk_scp_ipi.c
index cd0b601..4ef5491 100644
--- a/drivers/remoteproc/mtk_scp_ipi.c
+++ b/drivers/remoteproc/mtk_scp_ipi.c
@@ -162,10 +162,12 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id, void *buf, unsigned int len,
 	struct mtk_share_obj __iomem *send_obj = scp->send_buf;
 	u32 val;
 	int ret;
+	size_t share_buf_offset;
+	void __iomem *share_buf_io_address;
 
 	if (WARN_ON(id <= SCP_IPI_INIT) || WARN_ON(id >= SCP_IPI_MAX) ||
 	    WARN_ON(id == SCP_IPI_NS_SERVICE) ||
-	    WARN_ON(len > sizeof(send_obj->share_buf)) || WARN_ON(!buf))
+	    WARN_ON(len > scp->data->ipi_buffer_size) || WARN_ON(!buf))
 		return -EINVAL;
 
 	ret = clk_prepare_enable(scp->clk);
@@ -184,7 +186,10 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id, void *buf, unsigned int len,
 		goto unlock_mutex;
 	}
 
-	scp_memcpy_aligned(send_obj->share_buf, buf, len);
+	share_buf_offset = offsetof(struct mtk_share_obj, share_buf);
+	share_buf_io_address = (void __iomem *)((uintptr_t)scp->send_buf + share_buf_offset);
+
+	scp_memcpy_aligned(share_buf_io_address, buf, len);
 
 	writel(len, &send_obj->len);
 	writel(id, &send_obj->id);
diff --git a/include/linux/remoteproc/mtk_scp.h b/include/linux/remoteproc/mtk_scp.h
index 7c2b7cc9..344ff41 100644
--- a/include/linux/remoteproc/mtk_scp.h
+++ b/include/linux/remoteproc/mtk_scp.h
@@ -43,6 +43,7 @@ enum scp_ipi_id {
 	SCP_IPI_CROS_HOST_CMD,
 	SCP_IPI_VDEC_LAT,
 	SCP_IPI_VDEC_CORE,
+	SCP_IPI_IMGSYS_CMD,
 	SCP_IPI_NS_SERVICE = 0xFF,
 	SCP_IPI_MAX = 0x100,
 };
-- 
2.6.4

Re: [PATCH 1/2] dt-bindings: remoteproc: mediatek: Support MT8188 dual-core SCP

[Krzysztof Kozlowski]

On 11/04/2024 05:37, olivia.wen wrote:
> Under different applications, the MT8188 SCP can be used as single-core
> or dual-core.
> 
> Signed-off-by: olivia.wen <olivia.wen@mediatek.com>

Are you sure you use full name, not email login as name?

> ---
>  Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> index 507f98f..7e7b567 100644
> --- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> @@ -22,7 +22,7 @@ properties:
>        - mediatek,mt8192-scp
>        - mediatek,mt8195-scp
>        - mediatek,mt8195-scp-dual
> -
> +      - mediatek,mt8188-scp-dual

Missing blank line, misordered.


>    reg:
>      description:
>        Should contain the address ranges for memory regions SRAM, CFG, and,
> @@ -195,6 +195,7 @@ allOf:
>          compatible:
>            enum:
>              - mediatek,mt8195-scp-dual
> +            - mediatek,mt8188-scp-dual

Again, keep the order.



Best regards,
Krzysztof

Re: [PATCH 2/2] remoteproc: mediatek: Support MT8188 SCP core 1

[Krzysztof Kozlowski]

On 11/04/2024 05:37, olivia.wen wrote:
> +};
> +
>  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 },
> @@ -1323,6 +1362,7 @@ static const struct of_device_id mtk_scp_of_match[] = {
>  	{ .compatible = "mediatek,mt8192-scp", .data = &mt8192_of_data },
>  	{ .compatible = "mediatek,mt8195-scp", .data = &mt8195_of_data },
>  	{ .compatible = "mediatek,mt8195-scp-dual", .data = &mt8195_of_data_cores },
> +	{ .compatible = "mediatek,mt8188-scp-dual", .data = &mt8188_of_data_cores },

Why do you add new entries to the end? Look at the list first.

Best regards,
Krzysztof

Re: [PATCH 2/2] remoteproc: mediatek: Support MT8188 SCP core 1

[AngeloGioacchino Del Regno]

Il 11/04/24 05:37, olivia.wen ha scritto:
> To Support MT8188 SCP core 1 for ISP driver.
> The SCP on different chips will require different code sizes
>   and IPI buffer sizes based on varying requirements.
> 
> Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
> ---
>   drivers/remoteproc/mtk_common.h    |  5 +--
>   drivers/remoteproc/mtk_scp.c       | 62 +++++++++++++++++++++++++++++++-------
>   drivers/remoteproc/mtk_scp_ipi.c   |  9 ++++--
>   include/linux/remoteproc/mtk_scp.h |  1 +
>   4 files changed, 62 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
> index 6d7736a..8f37f65 100644
> --- a/drivers/remoteproc/mtk_common.h
> +++ b/drivers/remoteproc/mtk_common.h
> @@ -78,7 +78,6 @@
>   #define MT8195_L2TCM_OFFSET			0x850d0
>   
>   #define SCP_FW_VER_LEN			32
> -#define SCP_SHARE_BUFFER_SIZE		288
>   
>   struct scp_run {
>   	u32 signaled;
> @@ -110,6 +109,8 @@ struct mtk_scp_of_data {
>   	u32 host_to_scp_int_bit;
>   
>   	size_t ipi_buf_offset;
> +	u32 ipi_buffer_size;

this should be `ipi_share_buf_size`

> +	u32 max_code_size;

max_code_size should probably be dram_code_size or max_dram_size or dram_size.

Also, both should be size_t, not u32.

>   };
>   
>   struct mtk_scp_of_cluster {
> @@ -162,7 +163,7 @@ struct mtk_scp {
>   struct mtk_share_obj {
>   	u32 id;
>   	u32 len;
> -	u8 share_buf[SCP_SHARE_BUFFER_SIZE];
> +	u8 *share_buf;
>   };
>   
>   void scp_memcpy_aligned(void __iomem *dst, const void *src, unsigned int len);
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index 6751829..270718d 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -20,7 +20,6 @@
>   #include "mtk_common.h"
>   #include "remoteproc_internal.h"
>   
> -#define MAX_CODE_SIZE 0x500000
>   #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
>   
>   /**
> @@ -94,14 +93,14 @@ static void scp_ipi_handler(struct mtk_scp *scp)
>   {
>   	struct mtk_share_obj __iomem *rcv_obj = scp->recv_buf;
>   	struct scp_ipi_desc *ipi_desc = scp->ipi_desc;
> -	u8 tmp_data[SCP_SHARE_BUFFER_SIZE];
> +	u8 *tmp_data;
>   	scp_ipi_handler_t handler;
>   	u32 id = readl(&rcv_obj->id);
>   	u32 len = readl(&rcv_obj->len);
>   
> -	if (len > SCP_SHARE_BUFFER_SIZE) {
> +	if (len > scp->data->ipi_buffer_size) {
>   		dev_err(scp->dev, "ipi message too long (len %d, max %d)", len,
> -			SCP_SHARE_BUFFER_SIZE);
> +			scp->data->ipi_buffer_size);
>   		return;
>   	}
>   	if (id >= SCP_IPI_MAX) {
> @@ -109,6 +108,10 @@ static void scp_ipi_handler(struct mtk_scp *scp)
>   		return;
>   	}
>   
> +	tmp_data = kzalloc(len, GFP_KERNEL);

I think that this will be impacting on performance a bit, especially if
the scp_ipi_handler gets called frequently (and also remember that this
is in interrupt context).

For best performance, you should allocate this at probe time (in struct mtk_scp
or somewhere else), then:

len = ipi message length
memset zero the tmp_data from len to ipi_buffer_size

memcpy_fromio(....) etc

> +	if (!tmp_data)
> +		return;
> +
>   	scp_ipi_lock(scp, id);
>   	handler = ipi_desc[id].handler;
>   	if (!handler) {
> @@ -123,6 +126,7 @@ static void scp_ipi_handler(struct mtk_scp *scp)
>   
>   	scp->ipi_id_ack[id] = true;
>   	wake_up(&scp->ack_wq);
> +	kfree(tmp_data);

There's a possible memory leak. You forgot to kfree in the NULL handler path.

>   }
>   
>   static int scp_elf_read_ipi_buf_addr(struct mtk_scp *scp,
> @@ -133,6 +137,7 @@ static int scp_ipi_init(struct mtk_scp *scp, const struct firmware *fw)
>   {
>   	int ret;
>   	size_t buf_sz, offset;
> +	size_t share_buf_offset;
>   
>   	/* read the ipi buf addr from FW itself first */
>   	ret = scp_elf_read_ipi_buf_addr(scp, fw, &offset);
> @@ -154,10 +159,12 @@ static int scp_ipi_init(struct mtk_scp *scp, const struct firmware *fw)
>   
>   	scp->recv_buf = (struct mtk_share_obj __iomem *)
>   			(scp->sram_base + offset);
> +	share_buf_offset = sizeof(scp->recv_buf->id)
> +		+ sizeof(scp->recv_buf->len) + scp->data->ipi_buffer_size;
>   	scp->send_buf = (struct mtk_share_obj __iomem *)
> -			(scp->sram_base + offset + sizeof(*scp->recv_buf));
> -	memset_io(scp->recv_buf, 0, sizeof(*scp->recv_buf));
> -	memset_io(scp->send_buf, 0, sizeof(*scp->send_buf));
> +			(scp->sram_base + offset + share_buf_offset);
> +	memset_io(scp->recv_buf, 0, share_buf_offset);
> +	memset_io(scp->send_buf, 0, share_buf_offset);
>   
>   	return 0;
>   }
> @@ -891,7 +898,7 @@ static int scp_map_memory_region(struct mtk_scp *scp)
>   	}
>   
>   	/* Reserved SCP code size */
> -	scp->dram_size = MAX_CODE_SIZE;
> +	scp->dram_size = scp->data->max_code_size;

Remove the dram_size member from struct mtk_scp and use max_code_size directly.

>   	scp->cpu_addr = dma_alloc_coherent(scp->dev, scp->dram_size,
>   					   &scp->dma_addr, GFP_KERNEL);
>   	if (!scp->cpu_addr)
> @@ -1247,6 +1254,8 @@ static const struct mtk_scp_of_data mt8183_of_data = {
>   	.host_to_scp_reg = MT8183_HOST_TO_SCP,
>   	.host_to_scp_int_bit = MT8183_HOST_IPC_INT_BIT,
>   	.ipi_buf_offset = 0x7bdb0,
> +	.max_code_size = 0x500000,
> +	.ipi_buffer_size = 288,
>   };
>   
>   static const struct mtk_scp_of_data mt8186_of_data = {
> @@ -1260,18 +1269,22 @@ static const struct mtk_scp_of_data mt8186_of_data = {
>   	.host_to_scp_reg = MT8183_HOST_TO_SCP,
>   	.host_to_scp_int_bit = MT8183_HOST_IPC_INT_BIT,
>   	.ipi_buf_offset = 0x3bdb0,
> +	.max_code_size = 0x500000,
> +	.ipi_buffer_size = 288,
>   };
>   
>   static const struct mtk_scp_of_data mt8188_of_data = {
>   	.scp_clk_get = mt8195_scp_clk_get,
> -	.scp_before_load = mt8192_scp_before_load,
> -	.scp_irq_handler = mt8192_scp_irq_handler,
> +	.scp_before_load = mt8195_scp_before_load,
> +	.scp_irq_handler = mt8195_scp_irq_handler,

You should mention the reason of this change in the commit description, or better,
you should make a separate commit with a Fixes tag for this.

>   	.scp_reset_assert = mt8192_scp_reset_assert,
>   	.scp_reset_deassert = mt8192_scp_reset_deassert,
> -	.scp_stop = mt8192_scp_stop,
> +	.scp_stop = mt8195_scp_stop,
>   	.scp_da_to_va = mt8192_scp_da_to_va,
>   	.host_to_scp_reg = MT8192_GIPC_IN_SET,
>   	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
> +	.max_code_size = 0x500000,
> +	.ipi_buffer_size = 600,
>   };
>   
>   static const struct mtk_scp_of_data mt8192_of_data = {
> @@ -1284,6 +1297,8 @@ static const struct mtk_scp_of_data mt8192_of_data = {
>   	.scp_da_to_va = mt8192_scp_da_to_va,
>   	.host_to_scp_reg = MT8192_GIPC_IN_SET,
>   	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
> +	.max_code_size = 0x500000,
> +	.ipi_buffer_size = 288,
>   };
>   
>   static const struct mtk_scp_of_data mt8195_of_data = {
> @@ -1296,6 +1311,8 @@ static const struct mtk_scp_of_data mt8195_of_data = {
>   	.scp_da_to_va = mt8192_scp_da_to_va,
>   	.host_to_scp_reg = MT8192_GIPC_IN_SET,
>   	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
> +	.max_code_size = 0x500000,
> +	.ipi_buffer_size = 288,
>   };
>   
>   static const struct mtk_scp_of_data mt8195_of_data_c1 = {
> @@ -1308,6 +1325,22 @@ static const struct mtk_scp_of_data mt8195_of_data_c1 = {
>   	.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,
> +	.max_code_size = 0x500000,
> +	.ipi_buffer_size = 288,
> +};
> +
> +static const struct mtk_scp_of_data mt8188_of_data_c1 = {
> +	.scp_clk_get = mt8195_scp_clk_get,
> +	.scp_before_load = mt8195_scp_c1_before_load,
> +	.scp_irq_handler = mt8195_scp_c1_irq_handler,
> +	.scp_reset_assert = mt8195_scp_c1_reset_assert,
> +	.scp_reset_deassert = mt8195_scp_c1_reset_deassert,
> +	.scp_stop = mt8195_scp_c1_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,
> +	.max_code_size = 0xA00000,
> +	.ipi_buffer_size = 600,

I wonder if it's more sensible to add a new struct instead,
so that you can define

static const struct mtk_scp_sizes_data mt8188_scp_sizes = {
	.max_code_size = 0xA00000,
	.ipi_buffer_size = 600
};

...and then reuse like

static const struct mtk_scp_of_data mt8188_of_data_c1 = {
	..... stuff .....
	.scp_sizes = &mt8188_scp_sizes
};

...that'd be more important for mt8192, 95 and the others as those params
would be reused many, many times.

>   };
>   
>   static const struct mtk_scp_of_data *mt8195_of_data_cores[] = {
> @@ -1316,6 +1349,12 @@ static const struct mtk_scp_of_data *mt8195_of_data_cores[] = {
>   	NULL
>   };
>   
> +static const struct mtk_scp_of_data *mt8188_of_data_cores[] = {
> +	&mt8188_of_data,
> +	&mt8188_of_data_c1,
> +	NULL
> +};
> +
>   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 },
> @@ -1323,6 +1362,7 @@ static const struct of_device_id mtk_scp_of_match[] = {
>   	{ .compatible = "mediatek,mt8192-scp", .data = &mt8192_of_data },
>   	{ .compatible = "mediatek,mt8195-scp", .data = &mt8195_of_data },
>   	{ .compatible = "mediatek,mt8195-scp-dual", .data = &mt8195_of_data_cores },
> +	{ .compatible = "mediatek,mt8188-scp-dual", .data = &mt8188_of_data_cores },
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, mtk_scp_of_match);
> diff --git a/drivers/remoteproc/mtk_scp_ipi.c b/drivers/remoteproc/mtk_scp_ipi.c
> index cd0b601..4ef5491 100644
> --- a/drivers/remoteproc/mtk_scp_ipi.c
> +++ b/drivers/remoteproc/mtk_scp_ipi.c
> @@ -162,10 +162,12 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id, void *buf, unsigned int len,
>   	struct mtk_share_obj __iomem *send_obj = scp->send_buf;
>   	u32 val;
>   	int ret;
> +	size_t share_buf_offset;
> +	void __iomem *share_buf_io_address;
>   
>   	if (WARN_ON(id <= SCP_IPI_INIT) || WARN_ON(id >= SCP_IPI_MAX) ||
>   	    WARN_ON(id == SCP_IPI_NS_SERVICE) ||
> -	    WARN_ON(len > sizeof(send_obj->share_buf)) || WARN_ON(!buf))
> +	    WARN_ON(len > scp->data->ipi_buffer_size) || WARN_ON(!buf))
>   		return -EINVAL;
>   
>   	ret = clk_prepare_enable(scp->clk);
> @@ -184,7 +186,10 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id, void *buf, unsigned int len,
>   		goto unlock_mutex;
>   	}
>   
> -	scp_memcpy_aligned(send_obj->share_buf, buf, len);
> +	share_buf_offset = offsetof(struct mtk_share_obj, share_buf);
> +	share_buf_io_address = (void __iomem *)((uintptr_t)scp->send_buf + share_buf_offset);
> +
> +	scp_memcpy_aligned(share_buf_io_address, buf, len);
>   
>   	writel(len, &send_obj->len);
>   	writel(id, &send_obj->id);
> diff --git a/include/linux/remoteproc/mtk_scp.h b/include/linux/remoteproc/mtk_scp.h
> index 7c2b7cc9..344ff41 100644
> --- a/include/linux/remoteproc/mtk_scp.h
> +++ b/include/linux/remoteproc/mtk_scp.h
> @@ -43,6 +43,7 @@ enum scp_ipi_id {
>   	SCP_IPI_CROS_HOST_CMD,
>   	SCP_IPI_VDEC_LAT,
>   	SCP_IPI_VDEC_CORE,
> +	SCP_IPI_IMGSYS_CMD,

There's no mention of the addition of this new IPI ID in the commit description.
Please write something about it.

Cheers,
Angelo

>   	SCP_IPI_NS_SERVICE = 0xFF,
>   	SCP_IPI_MAX = 0x100,
>   };

Re: [PATCH 1/2] dt-bindings: remoteproc: mediatek: Support MT8188 dual-core SCP

[AngeloGioacchino Del Regno]

Il 11/04/24 05:37, olivia.wen ha scritto:
> Under different applications, the MT8188 SCP can be used as single-core
> or dual-core.
> 
> Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
> ---
>   Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> index 507f98f..7e7b567 100644
> --- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> @@ -22,7 +22,7 @@ properties:
>         - mediatek,mt8192-scp
>         - mediatek,mt8195-scp
>         - mediatek,mt8195-scp-dual
> -

Don't remove the blank line, it's there for readability.

> +      - mediatek,mt8188-scp-dual

After addressing that comment,

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

>     reg:
>       description:
>         Should contain the address ranges for memory regions SRAM, CFG, and,
> @@ -195,6 +195,7 @@ allOf:
>           compatible:
>             enum:
>               - mediatek,mt8195-scp-dual
> +            - mediatek,mt8188-scp-dual
>       then:
>         properties:
>           reg:

Re: [PATCH 1/2] dt-bindings: remoteproc: mediatek: Support MT8188 dual-core SCP

[AngeloGioacchino Del Regno]

Il 11/04/24 09:34, AngeloGioacchino Del Regno ha scritto:
> Il 11/04/24 05:37, olivia.wen ha scritto:
>> Under different applications, the MT8188 SCP can be used as single-core
>> or dual-core.
>>
>> Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
>> ---
>>   Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml 
>> b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
>> index 507f98f..7e7b567 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
>> +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
>> @@ -22,7 +22,7 @@ properties:
>>         - mediatek,mt8192-scp
>>         - mediatek,mt8195-scp
>>         - mediatek,mt8195-scp-dual
>> -
> 
> Don't remove the blank line, it's there for readability.
> 
>> +      - mediatek,mt8188-scp-dual

Ah, sorry, one more comment. Please, keep the entries ordered by name.
8188 goes before 8195.

> 
> After addressing that comment,
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
>>     reg:
>>       description:
>>         Should contain the address ranges for memory regions SRAM, CFG, and,
>> @@ -195,6 +195,7 @@ allOf:
>>           compatible:
>>             enum:
>>               - mediatek,mt8195-scp-dual
>> +            - mediatek,mt8188-scp-dual

same here.

>>       then:
>>         properties:
>>           reg:
> 
> 

Re: [PATCH 1/2] dt-bindings: remoteproc: mediatek: Support MT8188 dual-core SCP

[Olivia Wen (温倩苓)]

Hi AngeloGioacchino,


On Thu, 2024-04-11 at 09:39 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/04/24 09:34, AngeloGioacchino Del Regno ha scritto:
> > Il 11/04/24 05:37, olivia.wen ha scritto:
> > > Under different applications, the MT8188 SCP can be used as
> > > single-core
> > > or dual-core.
> > > 
> > > Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
> > > ---
> > >   Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml | 3
> > > ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml 
> > > b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > index 507f98f..7e7b567 100644
> > > --- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > @@ -22,7 +22,7 @@ properties:
> > >         - mediatek,mt8192-scp
> > >         - mediatek,mt8195-scp
> > >         - mediatek,mt8195-scp-dual
> > > -
> > 
> > Don't remove the blank line, it's there for readability.
> > 
> > > +      - mediatek,mt8188-scp-dual
> 
> Ah, sorry, one more comment. Please, keep the entries ordered by
> name.
> 8188 goes before 8195.
> 
> > 
> > After addressing that comment,
> > 
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > 
> > >     reg:
> > >       description:
> > >         Should contain the address ranges for memory regions
> > > SRAM, CFG, and,
> > > @@ -195,6 +195,7 @@ allOf:
> > >           compatible:
> > >             enum:
> > >               - mediatek,mt8195-scp-dual
> > > +            - mediatek,mt8188-scp-dual
> 
> same here.
> 
> > >       then:
> > >         properties:
> > >           reg:
> > 
> > 
> 

Thanks for the reviews.
It will be corrected in the next version.

Best regards,
Olivia

Re: [PATCH 2/2] remoteproc: mediatek: Support MT8188 SCP core 1

[Olivia Wen (温倩苓)]

Hi AngeloGioacchino,

Thanks for the reviews.

On Thu, 2024-04-11 at 09:33 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/04/24 05:37, olivia.wen ha scritto:
> > To Support MT8188 SCP core 1 for ISP driver.
> > The SCP on different chips will require different code sizes
> >   and IPI buffer sizes based on varying requirements.
> > 
> > Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
> > ---
> >   drivers/remoteproc/mtk_common.h    |  5 +--
> >   drivers/remoteproc/mtk_scp.c       | 62
> > +++++++++++++++++++++++++++++++-------
> >   drivers/remoteproc/mtk_scp_ipi.c   |  9 ++++--
> >   include/linux/remoteproc/mtk_scp.h |  1 +
> >   4 files changed, 62 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/mtk_common.h
> > b/drivers/remoteproc/mtk_common.h
> > index 6d7736a..8f37f65 100644
> > --- a/drivers/remoteproc/mtk_common.h
> > +++ b/drivers/remoteproc/mtk_common.h
> > @@ -78,7 +78,6 @@
> >   #define MT8195_L2TCM_OFFSET			0x850d0
> >   
> >   #define SCP_FW_VER_LEN			32
> > -#define SCP_SHARE_BUFFER_SIZE		288
> >   
> >   struct scp_run {
> >   	u32 signaled;
> > @@ -110,6 +109,8 @@ struct mtk_scp_of_data {
> >   	u32 host_to_scp_int_bit;
> >   
> >   	size_t ipi_buf_offset;
> > +	u32 ipi_buffer_size;
> 
> this should be `ipi_share_buf_size`
> 
> > +	u32 max_code_size;
> 
> max_code_size should probably be dram_code_size or max_dram_size or
> dram_size.
> 
> Also, both should be size_t, not u32.
> 

It will be fixed in the next version.


> >   };
> >   
> >   struct mtk_scp_of_cluster {
> > @@ -162,7 +163,7 @@ struct mtk_scp {
> >   struct mtk_share_obj {
> >   	u32 id;
> >   	u32 len;
> > -	u8 share_buf[SCP_SHARE_BUFFER_SIZE];
> > +	u8 *share_buf;
> >   };
> >   
> >   void scp_memcpy_aligned(void __iomem *dst, const void *src,
> > unsigned int len);
> > diff --git a/drivers/remoteproc/mtk_scp.c
> > b/drivers/remoteproc/mtk_scp.c
> > index 6751829..270718d 100644
> > --- a/drivers/remoteproc/mtk_scp.c
> > +++ b/drivers/remoteproc/mtk_scp.c
> > @@ -20,7 +20,6 @@
> >   #include "mtk_common.h"
> >   #include "remoteproc_internal.h"
> >   
> > -#define MAX_CODE_SIZE 0x500000
> >   #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
> >   
> >   /**
> > @@ -94,14 +93,14 @@ static void scp_ipi_handler(struct mtk_scp
> > *scp)
> >   {
> >   	struct mtk_share_obj __iomem *rcv_obj = scp->recv_buf;
> >   	struct scp_ipi_desc *ipi_desc = scp->ipi_desc;
> > -	u8 tmp_data[SCP_SHARE_BUFFER_SIZE];
> > +	u8 *tmp_data;
> >   	scp_ipi_handler_t handler;
> >   	u32 id = readl(&rcv_obj->id);
> >   	u32 len = readl(&rcv_obj->len);
> >   
> > -	if (len > SCP_SHARE_BUFFER_SIZE) {
> > +	if (len > scp->data->ipi_buffer_size) {
> >   		dev_err(scp->dev, "ipi message too long (len %d, max
> > %d)", len,
> > -			SCP_SHARE_BUFFER_SIZE);
> > +			scp->data->ipi_buffer_size);
> >   		return;
> >   	}
> >   	if (id >= SCP_IPI_MAX) {
> > @@ -109,6 +108,10 @@ static void scp_ipi_handler(struct mtk_scp
> > *scp)
> >   		return;
> >   	}
> >   
> > +	tmp_data = kzalloc(len, GFP_KERNEL);
> 
> I think that this will be impacting on performance a bit, especially
> if
> the scp_ipi_handler gets called frequently (and also remember that
> this
> is in interrupt context).
> 
> For best performance, you should allocate this at probe time (in
> struct mtk_scp
> or somewhere else), then:
> 
> len = ipi message length
> memset zero the tmp_data from len to ipi_buffer_size
> 
> memcpy_fromio(....) etc
> 
> > +	if (!tmp_data)
> > +		return;
> > +
> >   	scp_ipi_lock(scp, id);
> >   	handler = ipi_desc[id].handler;
> >   	if (!handler) {
> > @@ -123,6 +126,7 @@ static void scp_ipi_handler(struct mtk_scp
> > *scp)
> >   
> >   	scp->ipi_id_ack[id] = true;
> >   	wake_up(&scp->ack_wq);
> > +	kfree(tmp_data);
> 
> There's a possible memory leak. You forgot to kfree in the NULL
> handler path.
> 
> >   }
> >   

It seems more appropriate to allocate memory in the function
scp_rproc_init and free memory within the function scp_free.

And memset zero the tmp_data by ipi_share_buffer_size in function
scp_ipi_handler.

I will make changes like this.
If there are any other suggestions, plese provide them.
Thank you.


> >   static int scp_elf_read_ipi_buf_addr(struct mtk_scp *scp,
> > @@ -133,6 +137,7 @@ static int scp_ipi_init(struct mtk_scp *scp,
> > const struct firmware *fw)
> >   {
> >   	int ret;
> >   	size_t buf_sz, offset;
> > +	size_t share_buf_offset;
> >   
> >   	/* read the ipi buf addr from FW itself first */
> >   	ret = scp_elf_read_ipi_buf_addr(scp, fw, &offset);
> > @@ -154,10 +159,12 @@ static int scp_ipi_init(struct mtk_scp *scp,
> > const struct firmware *fw)
> >   
> >   	scp->recv_buf = (struct mtk_share_obj __iomem *)
> >   			(scp->sram_base + offset);
> > +	share_buf_offset = sizeof(scp->recv_buf->id)
> > +		+ sizeof(scp->recv_buf->len) + scp->data-
> > >ipi_buffer_size;
> >   	scp->send_buf = (struct mtk_share_obj __iomem *)
> > -			(scp->sram_base + offset + sizeof(*scp-
> > >recv_buf));
> > -	memset_io(scp->recv_buf, 0, sizeof(*scp->recv_buf));
> > -	memset_io(scp->send_buf, 0, sizeof(*scp->send_buf));
> > +			(scp->sram_base + offset + share_buf_offset);
> > +	memset_io(scp->recv_buf, 0, share_buf_offset);
> > +	memset_io(scp->send_buf, 0, share_buf_offset);
> >   
> >   	return 0;
> >   }
> > @@ -891,7 +898,7 @@ static int scp_map_memory_region(struct mtk_scp
> > *scp)
> >   	}
> >   
> >   	/* Reserved SCP code size */
> > -	scp->dram_size = MAX_CODE_SIZE;
> > +	scp->dram_size = scp->data->max_code_size;
> 
> Remove the dram_size member from struct mtk_scp and use max_code_size
> directly.
> 

It will be corrected in the next version.

> >   	scp->cpu_addr = dma_alloc_coherent(scp->dev, scp->dram_size,
> >   					   &scp->dma_addr, GFP_KERNEL);
> >   	if (!scp->cpu_addr)
> > @@ -1247,6 +1254,8 @@ static const struct mtk_scp_of_data
> > mt8183_of_data = {
> >   	.host_to_scp_reg = MT8183_HOST_TO_SCP,
> >   	.host_to_scp_int_bit = MT8183_HOST_IPC_INT_BIT,
> >   	.ipi_buf_offset = 0x7bdb0,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 288,
> >   };
> >   
> >   static const struct mtk_scp_of_data mt8186_of_data = {
> > @@ -1260,18 +1269,22 @@ static const struct mtk_scp_of_data
> > mt8186_of_data = {
> >   	.host_to_scp_reg = MT8183_HOST_TO_SCP,
> >   	.host_to_scp_int_bit = MT8183_HOST_IPC_INT_BIT,
> >   	.ipi_buf_offset = 0x3bdb0,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 288,
> >   };
> >   
> >   static const struct mtk_scp_of_data mt8188_of_data = {
> >   	.scp_clk_get = mt8195_scp_clk_get,
> > -	.scp_before_load = mt8192_scp_before_load,
> > -	.scp_irq_handler = mt8192_scp_irq_handler,
> > +	.scp_before_load = mt8195_scp_before_load,
> > +	.scp_irq_handler = mt8195_scp_irq_handler,
> 
> You should mention the reason of this change in the commit
> description, or better,
> you should make a separate commit with a Fixes tag for this.
> 

I will add the reason in the commit description in the next version.

> >   	.scp_reset_assert = mt8192_scp_reset_assert,
> >   	.scp_reset_deassert = mt8192_scp_reset_deassert,
> > -	.scp_stop = mt8192_scp_stop,
> > +	.scp_stop = mt8195_scp_stop,
> >   	.scp_da_to_va = mt8192_scp_da_to_va,
> >   	.host_to_scp_reg = MT8192_GIPC_IN_SET,
> >   	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 600,
> >   };
> >   
> >   static const struct mtk_scp_of_data mt8192_of_data = {
> > @@ -1284,6 +1297,8 @@ static const struct mtk_scp_of_data
> > mt8192_of_data = {
> >   	.scp_da_to_va = mt8192_scp_da_to_va,
> >   	.host_to_scp_reg = MT8192_GIPC_IN_SET,
> >   	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 288,
> >   };
> >   
> >   static const struct mtk_scp_of_data mt8195_of_data = {
> > @@ -1296,6 +1311,8 @@ static const struct mtk_scp_of_data
> > mt8195_of_data = {
> >   	.scp_da_to_va = mt8192_scp_da_to_va,
> >   	.host_to_scp_reg = MT8192_GIPC_IN_SET,
> >   	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 288,
> >   };
> >   
> >   static const struct mtk_scp_of_data mt8195_of_data_c1 = {
> > @@ -1308,6 +1325,22 @@ static const struct mtk_scp_of_data
> > mt8195_of_data_c1 = {
> >   	.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,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 288,
> > +};
> > +
> > +static const struct mtk_scp_of_data mt8188_of_data_c1 = {
> > +	.scp_clk_get = mt8195_scp_clk_get,
> > +	.scp_before_load = mt8195_scp_c1_before_load,
> > +	.scp_irq_handler = mt8195_scp_c1_irq_handler,
> > +	.scp_reset_assert = mt8195_scp_c1_reset_assert,
> > +	.scp_reset_deassert = mt8195_scp_c1_reset_deassert,
> > +	.scp_stop = mt8195_scp_c1_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,
> > +	.max_code_size = 0xA00000,
> > +	.ipi_buffer_size = 600,
> 
> I wonder if it's more sensible to add a new struct instead,
> so that you can define
> 
> static const struct mtk_scp_sizes_data mt8188_scp_sizes = {
> 	.max_code_size = 0xA00000,
> 	.ipi_buffer_size = 600
> };
> 
> ...and then reuse like
> 
> static const struct mtk_scp_of_data mt8188_of_data_c1 = {
> 	..... stuff .....
> 	.scp_sizes = &mt8188_scp_sizes
> };
> 
> ...that'd be more important for mt8192, 95 and the others as those
> params
> would be reused many, many times.
> 

Thanks for this suggestion.
It will be modified in the next version.

> >   };
> >   
> >   static const struct mtk_scp_of_data *mt8195_of_data_cores[] = {
> > @@ -1316,6 +1349,12 @@ static const struct mtk_scp_of_data
> > *mt8195_of_data_cores[] = {
> >   	NULL
> >   };
> >   
> > +static const struct mtk_scp_of_data *mt8188_of_data_cores[] = {
> > +	&mt8188_of_data,
> > +	&mt8188_of_data_c1,
> > +	NULL
> > +};
> > +
> >   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
> > },
> > @@ -1323,6 +1362,7 @@ static const struct of_device_id
> > mtk_scp_of_match[] = {
> >   	{ .compatible = "mediatek,mt8192-scp", .data = &mt8192_of_data
> > },
> >   	{ .compatible = "mediatek,mt8195-scp", .data = &mt8195_of_data
> > },
> >   	{ .compatible = "mediatek,mt8195-scp-dual", .data =
> > &mt8195_of_data_cores },
> > +	{ .compatible = "mediatek,mt8188-scp-dual", .data =
> > &mt8188_of_data_cores },
> >   	{},
> >   };
> >   MODULE_DEVICE_TABLE(of, mtk_scp_of_match);
> > diff --git a/drivers/remoteproc/mtk_scp_ipi.c
> > b/drivers/remoteproc/mtk_scp_ipi.c
> > index cd0b601..4ef5491 100644
> > --- a/drivers/remoteproc/mtk_scp_ipi.c
> > +++ b/drivers/remoteproc/mtk_scp_ipi.c
> > @@ -162,10 +162,12 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id,
> > void *buf, unsigned int len,
> >   	struct mtk_share_obj __iomem *send_obj = scp->send_buf;
> >   	u32 val;
> >   	int ret;
> > +	size_t share_buf_offset;
> > +	void __iomem *share_buf_io_address;
> >   
> >   	if (WARN_ON(id <= SCP_IPI_INIT) || WARN_ON(id >= SCP_IPI_MAX)
> > ||
> >   	    WARN_ON(id == SCP_IPI_NS_SERVICE) ||
> > -	    WARN_ON(len > sizeof(send_obj->share_buf)) ||
> > WARN_ON(!buf))
> > +	    WARN_ON(len > scp->data->ipi_buffer_size) || WARN_ON(!buf))
> >   		return -EINVAL;
> >   
> >   	ret = clk_prepare_enable(scp->clk);
> > @@ -184,7 +186,10 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id,
> > void *buf, unsigned int len,
> >   		goto unlock_mutex;
> >   	}
> >   
> > -	scp_memcpy_aligned(send_obj->share_buf, buf, len);
> > +	share_buf_offset = offsetof(struct mtk_share_obj, share_buf);
> > +	share_buf_io_address = (void __iomem *)((uintptr_t)scp-
> > >send_buf + share_buf_offset);
> > +
> > +	scp_memcpy_aligned(share_buf_io_address, buf, len);
> >   
> >   	writel(len, &send_obj->len);
> >   	writel(id, &send_obj->id);
> > diff --git a/include/linux/remoteproc/mtk_scp.h
> > b/include/linux/remoteproc/mtk_scp.h
> > index 7c2b7cc9..344ff41 100644
> > --- a/include/linux/remoteproc/mtk_scp.h
> > +++ b/include/linux/remoteproc/mtk_scp.h
> > @@ -43,6 +43,7 @@ enum scp_ipi_id {
> >   	SCP_IPI_CROS_HOST_CMD,
> >   	SCP_IPI_VDEC_LAT,
> >   	SCP_IPI_VDEC_CORE,
> > +	SCP_IPI_IMGSYS_CMD,
> 
> There's no mention of the addition of this new IPI ID in the commit
> description.
> Please write something about it.

It will be added in the next version.

> Cheers,
> Angelo
> 
> >   	SCP_IPI_NS_SERVICE = 0xFF,
> >   	SCP_IPI_MAX = 0x100,
> >   };
> 
> 
> 

Best regards,
Olivia

Re: [PATCH 1/2] dt-bindings: remoteproc: mediatek: Support MT8188 dual-core SCP

[Olivia Wen (温倩苓)]

Hi Krzysztof,

Thanks for the reviews.

On Thu, 2024-04-11 at 08:06 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 11/04/2024 05:37, olivia.wen wrote:
> > Under different applications, the MT8188 SCP can be used as single-
> core
> > or dual-core.
> > 
> > Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
> 
> Are you sure you use full name, not email login as name?
> 

Thanks for the reminder. I have made changes.

> > ---
> >  Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git
> a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > index 507f98f..7e7b567 100644
> > --- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > @@ -22,7 +22,7 @@ properties:
> >        - mediatek,mt8192-scp
> >        - mediatek,mt8195-scp
> >        - mediatek,mt8195-scp-dual
> > -
> > +      - mediatek,mt8188-scp-dual
> 
> Missing blank line, misordered.
> 

It will be corrected in the next version.

> >    reg:
> >      description:
> >        Should contain the address ranges for memory regions SRAM,
> CFG, and,
> > @@ -195,6 +195,7 @@ allOf:
> >          compatible:
> >            enum:
> >              - mediatek,mt8195-scp-dual
> > +            - mediatek,mt8188-scp-dual
> 
> Again, keep the order.
> 

It will be corrected in the next version.

> Best regards,a
> Krzysztof
> 
> 

Best regards,
Olivia

Re: [PATCH 1/2] dt-bindings: remoteproc: mediatek: Support MT8188 dual-core SCP

[Olivia Wen (温倩苓)]

Hi AngeloGioacchino,

On Thu, 2024-04-11 at 09:39 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/04/24 09:34, AngeloGioacchino Del Regno ha scritto:
> > Il 11/04/24 05:37, olivia.wen ha scritto:
> > > Under different applications, the MT8188 SCP can be used as
> > > single-core
> > > or dual-core.
> > > 
> > > Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
> > > ---
> > >   Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml | 3
> > > ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml 
> > > b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > index 507f98f..7e7b567 100644
> > > --- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > @@ -22,7 +22,7 @@ properties:
> > >         - mediatek,mt8192-scp
> > >         - mediatek,mt8195-scp
> > >         - mediatek,mt8195-scp-dual
> > > -
> > 
> > Don't remove the blank line, it's there for readability.
> > 
> > > +      - mediatek,mt8188-scp-dual
> 
> Ah, sorry, one more comment. Please, keep the entries ordered by
> name.
> 8188 goes before 8195.
> 
> > 
> > After addressing that comment,
> > 
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > 
> > >     reg:
> > >       description:
> > >         Should contain the address ranges for memory regions
> > > SRAM, CFG, and,
> > > @@ -195,6 +195,7 @@ allOf:
> > >           compatible:
> > >             enum:
> > >               - mediatek,mt8195-scp-dual
> > > +            - mediatek,mt8188-scp-dual
> 
> same here.
> 
> > >       then:
> > >         properties:
> > >           reg:
> > 
> > 
> 

Thanks for the reviews.
It will be corrected in the next version.

Best regards,
Olivia

Re: [PATCH 2/2] remoteproc: mediatek: Support MT8188 SCP core 1

[Olivia Wen (温倩苓)]

Hi Krzysztof,

On Thu, 2024-04-11 at 08:07 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 11/04/2024 05:37, olivia.wen wrote:
> > +};
> > +
> >  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 },
> > @@ -1323,6 +1362,7 @@ static const struct of_device_id
> mtk_scp_of_match[] = {
> >  { .compatible = "mediatek,mt8192-scp", .data = &mt8192_of_data },
> >  { .compatible = "mediatek,mt8195-scp", .data = &mt8195_of_data },
> >  { .compatible = "mediatek,mt8195-scp-dual", .data =
> &mt8195_of_data_cores },
> > +{ .compatible = "mediatek,mt8188-scp-dual", .data =
> &mt8188_of_data_cores },
> 
> Why do you add new entries to the end? Look at the list first.
> 
> Best regards,
> Krzysztof

Thanks for the reviews.
I will change the order as follows.
> +{ .compatible = "mediatek,mt8188-scp-dual", .data =
&mt8188_of_data_cores },
>  { .compatible = "mediatek,mt8195-scp-dual", .data =
&mt8195_of_data_cores },

Best regards,
Olivia

Re: [PATCH 2/2] remoteproc: mediatek: Support MT8188 SCP core 1

[Olivia Wen (温倩苓)]

Hi AngeloGioacchino,

Thanks for the reviews.

On Thu, 2024-04-11 at 09:33 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/04/24 05:37, olivia.wen ha scritto:
> > To Support MT8188 SCP core 1 for ISP driver.
> > The SCP on different chips will require different code sizes
> >   and IPI buffer sizes based on varying requirements.
> > 
> > Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
> > ---
> >   drivers/remoteproc/mtk_common.h    |  5 +--
> >   drivers/remoteproc/mtk_scp.c       | 62
> > +++++++++++++++++++++++++++++++-------
> >   drivers/remoteproc/mtk_scp_ipi.c   |  9 ++++--
> >   include/linux/remoteproc/mtk_scp.h |  1 +
> >   4 files changed, 62 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/mtk_common.h
> > b/drivers/remoteproc/mtk_common.h
> > index 6d7736a..8f37f65 100644
> > --- a/drivers/remoteproc/mtk_common.h
> > +++ b/drivers/remoteproc/mtk_common.h
> > @@ -78,7 +78,6 @@
> >   #define MT8195_L2TCM_OFFSET			0x850d0
> >   
> >   #define SCP_FW_VER_LEN			32
> > -#define SCP_SHARE_BUFFER_SIZE		288
> >   
> >   struct scp_run {
> >   	u32 signaled;
> > @@ -110,6 +109,8 @@ struct mtk_scp_of_data {
> >   	u32 host_to_scp_int_bit;
> >   
> >   	size_t ipi_buf_offset;
> > +	u32 ipi_buffer_size;
> 
> this should be `ipi_share_buf_size`
> 
> > +	u32 max_code_size;
> 
> max_code_size should probably be dram_code_size or max_dram_size or
> dram_size.
> 
> Also, both should be size_t, not u32.

It will be fixed in the next version.

> 
> >   };
> >   
> >   struct mtk_scp_of_cluster {
> > @@ -162,7 +163,7 @@ struct mtk_scp {
> >   struct mtk_share_obj {
> >   	u32 id;
> >   	u32 len;
> > -	u8 share_buf[SCP_SHARE_BUFFER_SIZE];
> > +	u8 *share_buf;
> >   };
> >   
> >   void scp_memcpy_aligned(void __iomem *dst, const void *src,
> > unsigned int len);
> > diff --git a/drivers/remoteproc/mtk_scp.c
> > b/drivers/remoteproc/mtk_scp.c
> > index 6751829..270718d 100644
> > --- a/drivers/remoteproc/mtk_scp.c
> > +++ b/drivers/remoteproc/mtk_scp.c
> > @@ -20,7 +20,6 @@
> >   #include "mtk_common.h"
> >   #include "remoteproc_internal.h"
> >   
> > -#define MAX_CODE_SIZE 0x500000
> >   #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
> >   
> >   /**
> > @@ -94,14 +93,14 @@ static void scp_ipi_handler(struct mtk_scp
> > *scp)
> >   {
> >   	struct mtk_share_obj __iomem *rcv_obj = scp->recv_buf;
> >   	struct scp_ipi_desc *ipi_desc = scp->ipi_desc;
> > -	u8 tmp_data[SCP_SHARE_BUFFER_SIZE];
> > +	u8 *tmp_data;
> >   	scp_ipi_handler_t handler;
> >   	u32 id = readl(&rcv_obj->id);
> >   	u32 len = readl(&rcv_obj->len);
> >   
> > -	if (len > SCP_SHARE_BUFFER_SIZE) {
> > +	if (len > scp->data->ipi_buffer_size) {
> >   		dev_err(scp->dev, "ipi message too long (len %d, max
> > %d)", len,
> > -			SCP_SHARE_BUFFER_SIZE);
> > +			scp->data->ipi_buffer_size);
> >   		return;
> >   	}
> >   	if (id >= SCP_IPI_MAX) {
> > @@ -109,6 +108,10 @@ static void scp_ipi_handler(struct mtk_scp
> > *scp)
> >   		return;
> >   	}
> >   
> > +	tmp_data = kzalloc(len, GFP_KERNEL);
> 
> I think that this will be impacting on performance a bit, especially
> if
> the scp_ipi_handler gets called frequently (and also remember that
> this
> is in interrupt context).
> 
> For best performance, you should allocate this at probe time (in
> struct mtk_scp
> or somewhere else), then:
> 
> len = ipi message length
> memset zero the tmp_data from len to ipi_buffer_size
> 
> memcpy_fromio(....) etc
> 
> > +	if (!tmp_data)
> > +		return;
> > +
> >   	scp_ipi_lock(scp, id);
> >   	handler = ipi_desc[id].handler;
> >   	if (!handler) {
> > @@ -123,6 +126,7 @@ static void scp_ipi_handler(struct mtk_scp
> > *scp)
> >   
> >   	scp->ipi_id_ack[id] = true;
> >   	wake_up(&scp->ack_wq);
> > +	kfree(tmp_data);
> 
> There's a possible memory leak. You forgot to kfree in the NULL
> handler path.
> 
> >   }
> >   

It seems more appropriate to allocate memory in the function
scp_rproc_init and free memory within the function scp_free.

And memset zero the tmp_data by ipi_share_buffer_size in function
scp_ipi_handler.

I will make changes like this.
If there are any other suggestions, plese provide them.
Thank you.

> >   static int scp_elf_read_ipi_buf_addr(struct mtk_scp *scp,
> > @@ -133,6 +137,7 @@ static int scp_ipi_init(struct mtk_scp *scp,
> > const struct firmware *fw)
> >   {
> >   	int ret;
> >   	size_t buf_sz, offset;
> > +	size_t share_buf_offset;
> >   
> >   	/* read the ipi buf addr from FW itself first */
> >   	ret = scp_elf_read_ipi_buf_addr(scp, fw, &offset);
> > @@ -154,10 +159,12 @@ static int scp_ipi_init(struct mtk_scp *scp,
> > const struct firmware *fw)
> >   
> >   	scp->recv_buf = (struct mtk_share_obj __iomem *)
> >   			(scp->sram_base + offset);
> > +	share_buf_offset = sizeof(scp->recv_buf->id)
> > +		+ sizeof(scp->recv_buf->len) + scp->data-
> > >ipi_buffer_size;
> >   	scp->send_buf = (struct mtk_share_obj __iomem *)
> > -			(scp->sram_base + offset + sizeof(*scp-
> > >recv_buf));
> > -	memset_io(scp->recv_buf, 0, sizeof(*scp->recv_buf));
> > -	memset_io(scp->send_buf, 0, sizeof(*scp->send_buf));
> > +			(scp->sram_base + offset + share_buf_offset);
> > +	memset_io(scp->recv_buf, 0, share_buf_offset);
> > +	memset_io(scp->send_buf, 0, share_buf_offset);
> >   
> >   	return 0;
> >   }
> > @@ -891,7 +898,7 @@ static int scp_map_memory_region(struct mtk_scp
> > *scp)
> >   	}
> >   
> >   	/* Reserved SCP code size */
> > -	scp->dram_size = MAX_CODE_SIZE;
> > +	scp->dram_size = scp->data->max_code_size;
> 
> Remove the dram_size member from struct mtk_scp and use max_code_size
> directly.
> 

It will be corrected in the next version.

> >   	scp->cpu_addr = dma_alloc_coherent(scp->dev, scp->dram_size,
> >   					   &scp->dma_addr, GFP_KERNEL);
> >   	if (!scp->cpu_addr)
> > @@ -1247,6 +1254,8 @@ static const struct mtk_scp_of_data
> > mt8183_of_data = {
> >   	.host_to_scp_reg = MT8183_HOST_TO_SCP,
> >   	.host_to_scp_int_bit = MT8183_HOST_IPC_INT_BIT,
> >   	.ipi_buf_offset = 0x7bdb0,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 288,
> >   };
> >   
> >   static const struct mtk_scp_of_data mt8186_of_data = {
> > @@ -1260,18 +1269,22 @@ static const struct mtk_scp_of_data
> > mt8186_of_data = {
> >   	.host_to_scp_reg = MT8183_HOST_TO_SCP,
> >   	.host_to_scp_int_bit = MT8183_HOST_IPC_INT_BIT,
> >   	.ipi_buf_offset = 0x3bdb0,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 288,
> >   };
> >   
> >   static const struct mtk_scp_of_data mt8188_of_data = {
> >   	.scp_clk_get = mt8195_scp_clk_get,
> > -	.scp_before_load = mt8192_scp_before_load,
> > -	.scp_irq_handler = mt8192_scp_irq_handler,
> > +	.scp_before_load = mt8195_scp_before_load,
> > +	.scp_irq_handler = mt8195_scp_irq_handler,
> 
> You should mention the reason of this change in the commit
> description, or better,
> you should make a separate commit with a Fixes tag for this.
> 

I will add the reason in the commit description in the next version.

> >   	.scp_reset_assert = mt8192_scp_reset_assert,
> >   	.scp_reset_deassert = mt8192_scp_reset_deassert,
> > -	.scp_stop = mt8192_scp_stop,
> > +	.scp_stop = mt8195_scp_stop,
> >   	.scp_da_to_va = mt8192_scp_da_to_va,
> >   	.host_to_scp_reg = MT8192_GIPC_IN_SET,
> >   	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 600,
> >   };
> >   
> >   static const struct mtk_scp_of_data mt8192_of_data = {
> > @@ -1284,6 +1297,8 @@ static const struct mtk_scp_of_data
> > mt8192_of_data = {
> >   	.scp_da_to_va = mt8192_scp_da_to_va,
> >   	.host_to_scp_reg = MT8192_GIPC_IN_SET,
> >   	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 288,
> >   };
> >   
> >   static const struct mtk_scp_of_data mt8195_of_data = {
> > @@ -1296,6 +1311,8 @@ static const struct mtk_scp_of_data
> > mt8195_of_data = {
> >   	.scp_da_to_va = mt8192_scp_da_to_va,
> >   	.host_to_scp_reg = MT8192_GIPC_IN_SET,
> >   	.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 288,
> >   };
> >   
> >   static const struct mtk_scp_of_data mt8195_of_data_c1 = {
> > @@ -1308,6 +1325,22 @@ static const struct mtk_scp_of_data
> > mt8195_of_data_c1 = {
> >   	.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,
> > +	.max_code_size = 0x500000,
> > +	.ipi_buffer_size = 288,
> > +};
> > +
> > +static const struct mtk_scp_of_data mt8188_of_data_c1 = {
> > +	.scp_clk_get = mt8195_scp_clk_get,
> > +	.scp_before_load = mt8195_scp_c1_before_load,
> > +	.scp_irq_handler = mt8195_scp_c1_irq_handler,
> > +	.scp_reset_assert = mt8195_scp_c1_reset_assert,
> > +	.scp_reset_deassert = mt8195_scp_c1_reset_deassert,
> > +	.scp_stop = mt8195_scp_c1_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,
> > +	.max_code_size = 0xA00000,
> > +	.ipi_buffer_size = 600,
> 
> I wonder if it's more sensible to add a new struct instead,
> so that you can define
> 
> static const struct mtk_scp_sizes_data mt8188_scp_sizes = {
> 	.max_code_size = 0xA00000,
> 	.ipi_buffer_size = 600
> };
> 
> ...and then reuse like
> 
> static const struct mtk_scp_of_data mt8188_of_data_c1 = {
> 	..... stuff .....
> 	.scp_sizes = &mt8188_scp_sizes
> };
> 
> ...that'd be more important for mt8192, 95 and the others as those
> params
> would be reused many, many times.
> 

Thanks for this suggestion.
It will be modified in the next version.

> >   };
> >   
> >   static const struct mtk_scp_of_data *mt8195_of_data_cores[] = {
> > @@ -1316,6 +1349,12 @@ static const struct mtk_scp_of_data
> > *mt8195_of_data_cores[] = {
> >   	NULL
> >   };
> >   
> > +static const struct mtk_scp_of_data *mt8188_of_data_cores[] = {
> > +	&mt8188_of_data,
> > +	&mt8188_of_data_c1,
> > +	NULL
> > +};
> > +
> >   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
> > },
> > @@ -1323,6 +1362,7 @@ static const struct of_device_id
> > mtk_scp_of_match[] = {
> >   	{ .compatible = "mediatek,mt8192-scp", .data = &mt8192_of_data
> > },
> >   	{ .compatible = "mediatek,mt8195-scp", .data = &mt8195_of_data
> > },
> >   	{ .compatible = "mediatek,mt8195-scp-dual", .data =
> > &mt8195_of_data_cores },
> > +	{ .compatible = "mediatek,mt8188-scp-dual", .data =
> > &mt8188_of_data_cores },
> >   	{},
> >   };
> >   MODULE_DEVICE_TABLE(of, mtk_scp_of_match);
> > diff --git a/drivers/remoteproc/mtk_scp_ipi.c
> > b/drivers/remoteproc/mtk_scp_ipi.c
> > index cd0b601..4ef5491 100644
> > --- a/drivers/remoteproc/mtk_scp_ipi.c
> > +++ b/drivers/remoteproc/mtk_scp_ipi.c
> > @@ -162,10 +162,12 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id,
> > void *buf, unsigned int len,
> >   	struct mtk_share_obj __iomem *send_obj = scp->send_buf;
> >   	u32 val;
> >   	int ret;
> > +	size_t share_buf_offset;
> > +	void __iomem *share_buf_io_address;
> >   
> >   	if (WARN_ON(id <= SCP_IPI_INIT) || WARN_ON(id >= SCP_IPI_MAX)
> > ||
> >   	    WARN_ON(id == SCP_IPI_NS_SERVICE) ||
> > -	    WARN_ON(len > sizeof(send_obj->share_buf)) ||
> > WARN_ON(!buf))
> > +	    WARN_ON(len > scp->data->ipi_buffer_size) || WARN_ON(!buf))
> >   		return -EINVAL;
> >   
> >   	ret = clk_prepare_enable(scp->clk);
> > @@ -184,7 +186,10 @@ int scp_ipi_send(struct mtk_scp *scp, u32 id,
> > void *buf, unsigned int len,
> >   		goto unlock_mutex;
> >   	}
> >   
> > -	scp_memcpy_aligned(send_obj->share_buf, buf, len);
> > +	share_buf_offset = offsetof(struct mtk_share_obj, share_buf);
> > +	share_buf_io_address = (void __iomem *)((uintptr_t)scp-
> > >send_buf + share_buf_offset);
> > +
> > +	scp_memcpy_aligned(share_buf_io_address, buf, len);
> >   
> >   	writel(len, &send_obj->len);
> >   	writel(id, &send_obj->id);
> > diff --git a/include/linux/remoteproc/mtk_scp.h
> > b/include/linux/remoteproc/mtk_scp.h
> > index 7c2b7cc9..344ff41 100644
> > --- a/include/linux/remoteproc/mtk_scp.h
> > +++ b/include/linux/remoteproc/mtk_scp.h
> > @@ -43,6 +43,7 @@ enum scp_ipi_id {
> >   	SCP_IPI_CROS_HOST_CMD,
> >   	SCP_IPI_VDEC_LAT,
> >   	SCP_IPI_VDEC_CORE,
> > +	SCP_IPI_IMGSYS_CMD,
> 
> There's no mention of the addition of this new IPI ID in the commit
> description.
> Please write something about it.
> 

It will be added in the next version.

> Cheers,
> Angelo
> 
> >   	SCP_IPI_NS_SERVICE = 0xFF,
> >   	SCP_IPI_MAX = 0x100,
> >   };
> 
> 

Best regards,
Olivia

Re: [PATCH 1/2] dt-bindings: remoteproc: mediatek: Support MT8188 dual-core SCP

[Olivia Wen (温倩苓)]

Hi AngeloGioacchino,

On Thu, 2024-04-11 at 09:39 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/04/24 09:34, AngeloGioacchino Del Regno ha scritto:
> > Il 11/04/24 05:37, olivia.wen ha scritto:
> > > Under different applications, the MT8188 SCP can be used as
> > > single-core
> > > or dual-core.
> > > 
> > > Signed-off-by: olivia.wen <olivia.wen@mediatek.com>
> > > ---
> > >   Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml | 3
> > > ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml 
> > > b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > index 507f98f..7e7b567 100644
> > > --- a/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > +++ b/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml
> > > @@ -22,7 +22,7 @@ properties:
> > >         - mediatek,mt8192-scp
> > >         - mediatek,mt8195-scp
> > >         - mediatek,mt8195-scp-dual
> > > -
> > 
> > Don't remove the blank line, it's there for readability.
> > 
> > > +      - mediatek,mt8188-scp-dual
> 
> Ah, sorry, one more comment. Please, keep the entries ordered by
> name.
> 8188 goes before 8195.
> 
> > 
> > After addressing that comment,
> > 
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > 
> > >     reg:
> > >       description:
> > >         Should contain the address ranges for memory regions
> > > SRAM, CFG, and,
> > > @@ -195,6 +195,7 @@ allOf:
> > >           compatible:
> > >             enum:
> > >               - mediatek,mt8195-scp-dual
> > > +            - mediatek,mt8188-scp-dual
> 
> same here.
> 
> > >       then:
> > >         properties:
> > >           reg:
> > 
> > 
> 

Thanks for the reviews.
It will be corrected in the next version.

Best regards,
Olivia