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

[Chengci.Xu]

On Thu, 2022-07-28 at 14:58 +0800, Yong Wu wrote:
> 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 at 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 at 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

OK, rename thie item to 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.

If we send an invalid parameter or something unpredictable happened,
such as ATF does not have related code, this SMC call will returns an
error, which means SMI-larb's register may in a HW default state.

In MT8188 we still need write SMI_LARB_NONSEC_CON for the HW default
value will enable IOMMU when SMC call failed. If we not write this
register, bank select function will lost. Thus, all the master that
with IOMMU default enable will lost the high bit[33:32], and send an
IOVA of 0~4G to IOMMU. That is abnormal. 

So I think it's better to add some comment.

> 
> And, Could we ignore the fail result? 
> 

This failure should not be ignored. If some masters want to disable
IOMMU and use PA to access DRAM for special case, this failure will
left IOMMU in enable state and lead to translation fault.

So I will add a return value for this function.

> >  }
> >  
> >  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.
> 

OK, got it.

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