Re: [PATCH v3 2/3] memory: mtk-smi: Add enable IOMMU SMC command for MM master

[Yong Wu <yong.wu@mediatek.com>]

On Wed, 2022-07-27 at 18:45 +0800, Chengci.Xu wrote:
> For concerns about security, the register to enable/disable IOMMU of
> SMI LARB should only be configured in secure world. Thus, we add some
> SMC command for multimedia master to enable/disable MM IOMMU in ATF
> by
> setting the register of SMI LARB. This function is prepared for
> MT8188.
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
> ---
>  drivers/memory/mtk-smi.c                 | 11 +++++++++++
>  include/linux/soc/mediatek/mtk_sip_svc.h |  3 +++
>  include/soc/mediatek/smi.h               |  7 +++++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index d7cb7ead2ac7..41ce66c39123 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2015-2016 MediaTek Inc.
>   * Author: Yong Wu <yong.wu@mediatek.com>
>   */
> +#include <linux/arm-smccc.h>
>  #include <linux/clk.h>
>  #include <linux/component.h>
>  #include <linux/device.h>
> @@ -14,6 +15,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/soc/mediatek/mtk_sip_svc.h>
>  #include <soc/mediatek/smi.h>
>  #include <dt-bindings/memory/mt2701-larb-port.h>
>  #include <dt-bindings/memory/mtk-memory-port.h>
> @@ -89,6 +91,7 @@
>  #define MTK_SMI_FLAG_THRT_UPDATE	BIT(0)
>  #define MTK_SMI_FLAG_SW_FLAG		BIT(1)
>  #define MTK_SMI_FLAG_SLEEP_CTL		BIT(2)
> +#define MTK_SMI_FLAG_SEC_REG		BIT(3)

Rename more detailly. This is about the configuring port registers.

something like: MTK_SMI_FLAG_CFG_PORT_SEC_CTL

>  #define MTK_SMI_CAPS(flags, _x)		(!!((flags) & (_x)))
>  
>  struct mtk_smi_reg_pair {
> @@ -235,6 +238,7 @@ static void
> mtk_smi_larb_config_port_gen2_general(struct device *dev)
>  	struct mtk_smi_larb *larb = dev_get_drvdata(dev);
>  	u32 reg, flags_general = larb->larb_gen->flags_general;
>  	const u8 *larbostd = larb->larb_gen->ostd ? larb->larb_gen-
> >ostd[larb->larbid] : NULL;
> +	struct arm_smccc_res res;
>  	int i;
>  
>  	if (BIT(larb->larbid) & larb->larb_gen-
> >larb_direct_to_common_mask)
> @@ -259,6 +263,13 @@ static void
> mtk_smi_larb_config_port_gen2_general(struct device *dev)
>  		reg |= BANK_SEL(larb->bank[i]);
>  		writel(reg, larb->base + SMI_LARB_NONSEC_CON(i));
>  	}
> +
> +	if (MTK_SMI_CAPS(flags_general, MTK_SMI_FLAG_SEC_REG)) {
> +		arm_smccc_smc(MTK_SIP_KERNEL_IOMMU_CONTROL,
> IOMMU_ATF_CMD_CONFIG_SMI_LARB,
> +			      larb->larbid, (u32)(*larb->mmu), 0, 0, 0,
> 0, &res);
> +		if (res.a0 != 0)
> +			dev_err(dev, "Enable iommu fail, ret %ld\n",
> res.a0);
> +	}

In this case, Do we still need write SMI_LARB_NONSEC_CON above? If no,
  
      if (MTK_SMI_CAPS(flags_general, MTK_SMI_FLAG_SEC_REG)) {
          xx
          return;
      }
      and put this before updating SMI_LARB_NONSEC_CON.

If yes. we should add some comment.

And, Could we ignore the fail result? 

>  }
>  
>  static const u8 mtk_smi_larb_mt8195_ostd[][SMI_LARB_PORT_NR_MAX] = {
> diff --git a/include/linux/soc/mediatek/mtk_sip_svc.h
> b/include/linux/soc/mediatek/mtk_sip_svc.h
> index 082398e0cfb1..0761128b4354 100644
> --- a/include/linux/soc/mediatek/mtk_sip_svc.h
> +++ b/include/linux/soc/mediatek/mtk_sip_svc.h
> @@ -22,4 +22,7 @@
>  	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, MTK_SIP_SMC_CONVENTION,
> \
>  			   ARM_SMCCC_OWNER_SIP, fn_id)
>  
> +/* IOMMU related SMC call */
> +#define MTK_SIP_KERNEL_IOMMU_CONTROL	MTK_SIP_SMC_CMD(0x514)
> +
>  #endif
> diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h
> index 11f7d6b59642..8c781b7bd88d 100644
> --- a/include/soc/mediatek/smi.h
> +++ b/include/soc/mediatek/smi.h
> @@ -9,6 +9,13 @@
>  #include <linux/bitops.h>
>  #include <linux/device.h>
>  
> +/* IOMMU & SMI ATF CMD */
> +
> +enum IOMMU_ATF_CMD {
> +	IOMMU_ATF_CMD_CONFIG_SMI_LARB,		/* For mm master to
> en/disable iommu */
> +	IOMMU_ATF_CMD_COUNT,

IOMMU_ATF_CMD_MAX?
> +};
> +

This enum only is used in IOMMU and SMI. Put it below inside the macro
CONFIG_MTK_SMI.


>  #if IS_ENABLED(CONFIG_MTK_SMI)
>  
>  #define MTK_SMI_MMU_EN(port)	BIT(port)