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

[Mathieu Poirier <mathieu.poirier@linaro.org>]

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
>