Re: [PATCH 2/2] remoteproc/mediatek: support L1TCM

Mathieu Poirier
2021-01-06T23:16:13Z

Hi Shih,

On Mon, Dec 14, 2020 at 01:05:21PM +0800, Tzung-Bi Shih wrote:
> L1TCM is a high performance memory region in MT8192 SCP.
>
> Reads L1TCM memory region from DTS to determine if the machine supports.
> Loads L1TCM memory region to SCP sys if the firmware provides.
>
> Starts from MT8192 SCP, the firmware contains physical addresses for
> each memory region, for instance:
>
> Program Headers:
> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
> LOAD 0xXXXXXX 0xXXXXXXXX 0x10500000 0xXXXXX 0xXXXXX XXX 0xXXXX
> LOAD 0xXXXXXX 0xXXXXXXXX 0x10700000 0xXXXXX 0xXXXXX XXX 0xXXXX
> LOAD 0xXXXXXX 0xXXXXXXXX 0x50000000 0xXXXXX 0xXXXXX XXX 0xXXXX
>
> Kernel driver can use the "PhysAddr" (i.e. da in the da_to_va callbacks)
> to know the ELF segment belongs to which region.
>
> To backward compatible to MT8183 SCP, separates the da_to_va callbacks
> for new and legacy version.
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
> ---
> drivers/remoteproc/mtk_common.h | 5 +++
> drivers/remoteproc/mtk_scp.c | 54 +++++++++++++++++++++++++++++++--
> 2 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
> index 988edb4977c3..94bc54b224ee 100644
> --- a/drivers/remoteproc/mtk_common.h
> +++ b/drivers/remoteproc/mtk_common.h
> @@ -75,6 +75,7 @@ struct mtk_scp_of_data {
> void (*scp_reset_assert)(struct mtk_scp *scp);
> void (*scp_reset_deassert)(struct mtk_scp *scp);
> void (*scp_stop)(struct mtk_scp *scp);
> + void *(*scp_da_to_va)(struct mtk_scp *scp, u64 da, size_t len);
>
> u32 host_to_scp_reg;
> u32 host_to_scp_int_bit;
> @@ -89,6 +90,10 @@ struct mtk_scp {
> void __iomem *reg_base;
> void __iomem *sram_base;
> size_t sram_size;
> + phys_addr_t sram_phys;
> + void __iomem *l1tcm_base;
> + size_t l1tcm_size;
> + phys_addr_t l1tcm_phys;
>
> const struct mtk_scp_of_data *data;
>
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index e0c235690361..f025aba67abc 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -458,9 +458,8 @@ static int scp_start(struct rproc *rproc)
> return ret;
> }
>
> -static void *scp_da_to_va(struct rproc *rproc, u64 da, size_t len)
> +static void *mt8183_scp_da_to_va(struct mtk_scp *scp, u64 da, size_t len)
> {
> - struct mtk_scp *scp = (struct mtk_scp *)rproc->priv;
> int offset;
>
> if (da < scp->sram_size) {
> @@ -476,6 +475,42 @@ static void *scp_da_to_va(struct rproc *rproc, u64 da, size_t len)
> return NULL;
> }
>
> +static void *mt8192_scp_da_to_va(struct mtk_scp *scp, u64 da, size_t len)
> +{
> + int offset;
> +
> + if (da >= scp->sram_phys &&
> + (da + len) <= scp->sram_phys + scp->sram_size) {
> + offset = da - scp->sram_phys;
> + return (void __force *)scp->sram_base + offset;
> + }
> +
> + /* optional memory region */
> + if (scp->l1tcm_size &&
> + da >= scp->l1tcm_phys &&
> + (da + len) <= scp->l1tcm_phys + scp->l1tcm_size) {
> + offset = da - scp->l1tcm_phys;
> + return (void __force *)scp->l1tcm_base + offset;
> + }
> +
> + /* optional memory region */
> + if (scp->dram_size &&
> + da >= scp->dma_addr &&
> + (da + len) <= scp->dma_addr + scp->dram_size) {
> + offset = da - scp->dma_addr;
> + return scp->cpu_addr + offset;
> + }
> +
> + return NULL;
> +}
> +
> +static void *scp_da_to_va(struct rproc *rproc, u64 da, size_t len)
> +{
> + struct mtk_scp *scp = (struct mtk_scp *)rproc->priv;
> +
> + return scp->data->scp_da_to_va(scp, da, len);
> +}
> +
> static void mt8183_scp_stop(struct mtk_scp *scp)
> {
> /* Disable SCP watchdog */
> @@ -714,6 +749,19 @@ static int scp_probe(struct platform_device *pdev)
> goto free_rproc;
> }
> scp->sram_size = resource_size(res);
> + scp->sram_phys = res->start;
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm");
> + if (res) {

As far as I can tell the if() condition isn't needed since
platform_get_resource_byname() returns NULL on error and devm_ioremap_resource()
is capable of handling that condition. As such the code to parse "l1tcm" can be
the same as what is done for "sram".

With the above:

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

> + scp->l1tcm_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR((__force void *)scp->l1tcm_base)) {
> + dev_err(dev, "Failed to parse and map l1tcm memory\n");
> + ret = PTR_ERR((__force void *)scp->l1tcm_base);
> + goto free_rproc;
> + }
> + scp->l1tcm_size = resource_size(res);
> + scp->l1tcm_phys = res->start;
> + }
>
> mutex_init(&scp->send_lock);
> for (i = 0; i < SCP_IPI_MAX; i++)
> @@ -803,6 +851,7 @@ static const struct mtk_scp_of_data mt8183_of_data = {
> .scp_reset_assert = mt8183_scp_reset_assert,
> .scp_reset_deassert = mt8183_scp_reset_deassert,
> .scp_stop = mt8183_scp_stop,
> + .scp_da_to_va = mt8183_scp_da_to_va,
> .host_to_scp_reg = MT8183_HOST_TO_SCP,
> .host_to_scp_int_bit = MT8183_HOST_IPC_INT_BIT,
> .ipi_buf_offset = 0x7bdb0,
> @@ -814,6 +863,7 @@ static const struct mtk_scp_of_data mt8192_of_data = {
> .scp_reset_assert = mt8192_scp_reset_assert,
> .scp_reset_deassert = mt8192_scp_reset_deassert,
> .scp_stop = mt8192_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,
> };
> --
> 2.29.2.684.gfbc64c5ab5-goog
>