Re: [PATCH 0/2] Add SCP support for mt8186

Re: [PATCH 0/2] Add SCP support for mt8186

[Mathieu Poirier]

Hi Allen,

On Tue, Jan 04, 2022 at 07:47:04PM +0800, allen-kh.cheng wrote:
> From: Allen-KH Cheng <Allen-KH.Cheng@mediatek.com>
> 
> changes since v1:
> - use mt8192_power_on_sram() helper
> - add MT8186_SCP_L1_SRAM_PD_P1 in mtk_common.h
> - add MT8186_SCP_L1_SRAM_PD_P2 in mtk_common.h

It is quite hard to follow the evolution of this patchset.  The second revision
had a "v2" tag but this one doesn't, which can be confused with the patchset
sent on January 3rd.  Please send another one (v4) with:

1) A cover letter.
2) The right SoB.
3) The right version tag.

> 
> Based on v5.16-rc8

This should be based on rproc-next[1].

Thanks,
Mathieu

https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/log/?h=rproc-next

> 
> allen-kh.cheng (2):
>   dt-bindings: remoteproc: mediatek: Add binding for mt8186 scp
>   remoteproc: mediatek: Support mt8186 scp
> 
>  .../bindings/remoteproc/mtk,scp.yaml          |  1 +
>  drivers/remoteproc/mtk_common.h               |  3 ++
>  drivers/remoteproc/mtk_scp.c                  | 35 +++++++++++++++++++
>  3 files changed, 39 insertions(+)
> 
> -- 
> 2.18.0
> 

Re: [PATCH 2/2] remoteproc: mediatek: Support mt8186 scp

[AngeloGioacchino Del Regno]

Il 04/01/22 12:47, allen-kh.cheng ha scritto:
> Add SCP support for mt8186
> 
> Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>

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

Re: [PATCH 2/2] remoteproc: mediatek: Support mt8186 scp

[AngeloGioacchino Del Regno]

Il 03/01/22 08:58, allen-kh.cheng ha scritto:
> Add SCP support for mt8186
> 
> Signed-off-by: Allen-KH Cheng <allen-kh.cheng@mediatek.com>

Hello! Thanks for the patch!
However, there's something to improve...

> ---
>   drivers/remoteproc/mtk_scp.c | 39 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index 36e48cf58ed6..38eab6b86fd7 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -365,6 +365,31 @@ static int mt8183_scp_before_load(struct mtk_scp *scp)
>   	return 0;
>   }
>   
> +static int mt8186_scp_before_load(struct mtk_scp *scp)
> +{
> +	int i;
> +
> +	/* Clear SCP to host interrupt */
> +	writel(MT8183_SCP_IPC_INT_BIT, scp->reg_base + MT8183_SCP_TO_HOST);
> +
> +	/* Reset clocks before loading FW */
> +	writel(0x0, scp->reg_base + MT8183_SCP_CLK_SW_SEL);
> +	writel(0x0, scp->reg_base + MT8183_SCP_CLK_DIV_SEL);
> +

.............

> +	/* Turn on the power of SCP's SRAM before using it. Enable 1 block per time*/
> +	for (i = 31; i >= 0; i--)
> +		writel(GENMASK(i, 0), scp->reg_base + MT8183_SCP_SRAM_PDN);
> +	writel(0, scp->reg_base + MT8183_SCP_SRAM_PDN);
> +

.............

This snippet corresponds exactly to the mt8192_power_on_sram() helper:
please avoid code duplication by simply calling that function instead.

Thanks,
- Angelo