| Message ID | 20240411033750.6476-3-olivia.wen@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support MT8188 SCP core 1 | expand |
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
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, > };
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
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
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
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, };
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(-)