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

[Chengci.Xu]

On Thu, 2022-07-28 at 13:11 +0200, AngeloGioacchino Del Regno wrote:
> Il 27/07/22 12:45, Chengci.Xu ha scritto:
> > 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)
> >   #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);
> 
> This means that the system will eventually crash or anyway be
> unstable: in this
> case, we should not allow further interaction with the IOMMUs and/or
> SMI.
> 
> So, if you place this here, you will have to change this function to
> return
> something for the caller to take action.

Thanks, a new patch will add a return value for this function.
And, we will return -EINVAL when enable iommu failed.

> 
> > +	}
> >   }
> >   
> >   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 {
> 
> Why do you have an enumeration when you're defining just one command?
> Is it expected to have more?

Yes, we will have another command for INFRA IOMMU enable/disable.

> 
> Besides, the enum name should be lower case, and...

Thanks, got it.

> 
> > +	IOMMU_ATF_CMD_CONFIG_SMI_LARB,		/* For mm master to
> > en/disable iommu */
> > +	IOMMU_ATF_CMD_COUNT,
> 
> if IOMMU_ATF_CMD_COUNT means "end of this enumeration", please call
> it
> IOMMU_ATF_CMD_MAX instead.

Thanks, got it.

> 
> > +};
> > +
> >   #if IS_ENABLED(CONFIG_MTK_SMI)
> >   
> >   #define MTK_SMI_MMU_EN(port)	BIT(port)
> 
>