iommu/mediatek: Add SVP support for mt8188

iommu/mediatek: Add SVP support for mt8188

[Yong Wu]

This patchset adds support for SVP(Secure video play). We always use the
last bank(id:4) to map secure memory. But the detail mapping and control
information are in the TEE. In kernel, We just register the IRQ to report
specific information when secure translation fault occurs.

Base on v6.6-rc1.

Yong Wu (4):
  iommu/mediatek: Initialise the secure bank
  iommu/mediatek: Add irq handle for secure bank
  iommu/mediatek: Avoid access secure bank register in runtime_suspend
  iommu/mediatek: mt8188: Enable secure bank for MM IOMMU

 drivers/iommu/mtk_iommu.c  | 94 +++++++++++++++++++++++++++-----------
 include/soc/mediatek/smi.h |  1 +
 2 files changed, 69 insertions(+), 26 deletions(-)

-- 
2.18.0

[PATCH 1/4] iommu/mediatek: Initialise the secure bank

[Yong Wu]

The lastest IOMMU always have 5 banks, and we always use the last bank
(id:4) for the secure memory address translation. This patch add a new
flag (SECURE_BANK_ENABLE) for this feature.

For the secure bank, its kernel va "base" is not helpful since the
secure bank registers has already been protected and can only be accessed
in the secure world. But we still record its register base, because we need
use it to determine which IOMMU HW the translation fault happen in the
secure world.

Signed-off-by: Anan Sun <anan.sun@mediatek.com>
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/iommu/mtk_iommu.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 640275873a27..4a2cffb28c61 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -146,6 +146,7 @@
 #define TF_PORT_TO_ADDR_MT8173		BIT(18)
 #define INT_ID_PORT_WIDTH_6		BIT(19)
 #define CFG_IFA_MASTER_IN_ATF		BIT(20)
+#define SECURE_BANK_ENABLE		BIT(21)
 
 #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)	\
 				((((pdata)->flags) & (mask)) == (_x))
@@ -162,6 +163,8 @@
 #define MTK_IOMMU_GROUP_MAX	8
 #define MTK_IOMMU_BANK_MAX	5
 
+#define MTK_IOMMU_SEC_BANKID	4
+
 enum mtk_iommu_plat {
 	M4U_MT2712,
 	M4U_MT6779,
@@ -240,9 +243,13 @@ struct mtk_iommu_plat_data {
 };
 
 struct mtk_iommu_bank_data {
-	void __iomem			*base;
+	union {
+		void __iomem		*base;
+		phys_addr_t		sec_bank_base;
+	};
 	int				irq;
 	u8				id;
+	bool				is_secure;
 	struct device			*parent_dev;
 	struct mtk_iommu_data		*parent_data;
 	spinlock_t			tlb_lock; /* lock for tlb range flush */
@@ -1309,7 +1316,15 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 			continue;
 		bank = &data->bank[i];
 		bank->id = i;
-		bank->base = base + i * MTK_IOMMU_BANK_SZ;
+		if (MTK_IOMMU_HAS_FLAG(data->plat_data, SECURE_BANK_ENABLE) &&
+		    bank->id == MTK_IOMMU_SEC_BANKID) {
+			/* Record the secure bank base to indicate which iommu TF in sec world */
+			bank->sec_bank_base = res->start + i * MTK_IOMMU_BANK_SZ;
+			bank->is_secure = true;
+		} else {
+			bank->base = base + i * MTK_IOMMU_BANK_SZ;
+			bank->is_secure = false;
+		}
 		bank->m4u_dom = NULL;
 
 		bank->irq = platform_get_irq(pdev, i);
-- 
2.25.1

[PATCH 2/4] iommu/mediatek: Add irq handle for secure bank

[Yong Wu]

The secure bank registers are protected and can only be accessed in
the secure world, thus it won't be initialised in linux. In kernel, we only
need register the irq handle to report which iommu HW the secure
translation fault happen in.

We will enter the ATF to read the detail fault information. If ATF fail or
ATF isn't the debug load, we won't get the fault information, just print
a simple "secure bank fault" log.

Signed-off-by: Anan Sun <anan.sun@mediatek.com>
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/iommu/mtk_iommu.c  | 61 +++++++++++++++++++++++++++-----------
 include/soc/mediatek/smi.h |  1 +
 2 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 4a2cffb28c61..e1faf2339b9a 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -465,22 +465,38 @@ static irqreturn_t mtk_iommu_isr(int irq, void *dev_id)
 	struct mtk_iommu_data *data = bank->parent_data;
 	struct mtk_iommu_domain *dom = bank->m4u_dom;
 	unsigned int fault_larb = MTK_INVALID_LARBID, fault_port = 0, sub_comm = 0;
-	u32 int_state, regval, va34_32, pa34_32;
 	const struct mtk_iommu_plat_data *plat_data = data->plat_data;
+	u32 int_state = ~0, regval = 0, va34_32, pa34_32;
+	u64 fault_iova = ~0ULL, fault_pa = ~0ULL;
 	void __iomem *base = bank->base;
-	u64 fault_iova, fault_pa;
+	struct arm_smccc_res res;
 	bool layer, write;
 
-	/* Read error info from registers */
-	int_state = readl_relaxed(base + REG_MMU_FAULT_ST1);
-	if (int_state & F_REG_MMU0_FAULT_MASK) {
-		regval = readl_relaxed(base + REG_MMU0_INT_ID);
-		fault_iova = readl_relaxed(base + REG_MMU0_FAULT_VA);
-		fault_pa = readl_relaxed(base + REG_MMU0_INVLD_PA);
+	if (bank->is_secure) {
+		/* Enter to secure world to Read fault status if it is secure bank. */
+		arm_smccc_smc(MTK_SIP_KERNEL_IOMMU_CONTROL,
+			      IOMMU_ATF_CMD_GET_SECURE_IOMMU_STATUS,
+			      bank->sec_bank_base, 0, 0, 0, 0, 0, &res);
+		if (res.a0 == 0) {
+			fault_iova = res.a1;
+			fault_pa = res.a2;
+			regval = res.a3;
+		} else {
+			dev_err_ratelimited(bank->parent_dev, "secure bank fault\n");
+			goto tlb_flush_all;
+		}
 	} else {
-		regval = readl_relaxed(base + REG_MMU1_INT_ID);
-		fault_iova = readl_relaxed(base + REG_MMU1_FAULT_VA);
-		fault_pa = readl_relaxed(base + REG_MMU1_INVLD_PA);
+		/* Read error info from registers */
+		int_state = readl_relaxed(base + REG_MMU_FAULT_ST1);
+		if (int_state & F_REG_MMU0_FAULT_MASK) {
+			regval = readl_relaxed(base + REG_MMU0_INT_ID);
+			fault_iova = readl_relaxed(base + REG_MMU0_FAULT_VA);
+			fault_pa = readl_relaxed(base + REG_MMU0_INVLD_PA);
+		} else {
+			regval = readl_relaxed(base + REG_MMU1_INT_ID);
+			fault_iova = readl_relaxed(base + REG_MMU1_FAULT_VA);
+			fault_pa = readl_relaxed(base + REG_MMU1_INVLD_PA);
+		}
 	}
 	layer = fault_iova & F_MMU_FAULT_VA_LAYER_BIT;
 	write = fault_iova & F_MMU_FAULT_VA_WRITE_BIT;
@@ -515,16 +531,19 @@ static irqreturn_t mtk_iommu_isr(int irq, void *dev_id)
 			       write ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ)) {
 		dev_err_ratelimited(
 			bank->parent_dev,
-			"fault type=0x%x iova=0x%llx pa=0x%llx master=0x%x(larb=%d port=%d) layer=%d %s\n",
-			int_state, fault_iova, fault_pa, regval, fault_larb, fault_port,
+			"bank(%u) fault type=0x%x iova=0x%llx pa=0x%llx master=0x%x(larb=%d port=%d) layer=%d %s\n",
+			bank->id, int_state, fault_iova, fault_pa, regval, fault_larb, fault_port,
 			layer, write ? "write" : "read");
 	}
 
 	/* Interrupt clear */
-	regval = readl_relaxed(base + REG_MMU_INT_CONTROL0);
-	regval |= F_INT_CLR_BIT;
-	writel_relaxed(regval, base + REG_MMU_INT_CONTROL0);
+	if (!bank->is_secure) {
+		regval = readl_relaxed(base + REG_MMU_INT_CONTROL0);
+		regval |= F_INT_CLR_BIT;
+		writel_relaxed(regval, base + REG_MMU_INT_CONTROL0);
+	}
 
+tlb_flush_all:
 	mtk_iommu_tlb_flush_all(data);
 
 	return IRQ_HANDLED;
@@ -1333,6 +1352,14 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 		bank->parent_dev = dev;
 		bank->parent_data = data;
 		spin_lock_init(&bank->tlb_lock);
+
+		/* Secure bank is initialised in secure world. Only need register irq here */
+		if (!bank->is_secure)
+			continue;
+		ret = devm_request_irq(bank->parent_dev, bank->irq, mtk_iommu_isr,
+				       0, dev_name(bank->parent_dev), (void *)bank);
+		if (ret)
+			return ret;
 	} while (++i < banks_num);
 
 	if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_BCLK)) {
@@ -1426,7 +1453,7 @@ static void mtk_iommu_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	for (i = 0; i < data->plat_data->banks_num; i++) {
 		bank = &data->bank[i];
-		if (!bank->m4u_dom)
+		if (!bank->m4u_dom && !data->bank[i].is_secure)
 			continue;
 		devm_free_irq(&pdev->dev, bank->irq, bank);
 	}
diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h
index 000eb1cf68b7..294550240aa6 100644
--- a/include/soc/mediatek/smi.h
+++ b/include/soc/mediatek/smi.h
@@ -14,6 +14,7 @@
 enum iommu_atf_cmd {
 	IOMMU_ATF_CMD_CONFIG_SMI_LARB,		/* For mm master to en/disable iommu */
 	IOMMU_ATF_CMD_CONFIG_INFRA_IOMMU,	/* For infra master to enable iommu */
+	IOMMU_ATF_CMD_GET_SECURE_IOMMU_STATUS,	/* Get secure iommu translation fault status */
 	IOMMU_ATF_CMD_MAX,
 };
 
-- 
2.25.1

[PATCH 3/4] iommu/mediatek: Avoid access secure bank register in runtime_suspend

[Yong Wu]

The secure bank registers are protected and they can only be accessed
in secure world. Avoid access them in runtime_suspend.

For secure bank, the m4u_dom always is NULL, thus we don't need add
this checking in runtime_resume.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/iommu/mtk_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index e1faf2339b9a..24d7f5138f7b 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1473,7 +1473,7 @@ static int __maybe_unused mtk_iommu_runtime_suspend(struct device *dev)
 	reg->ctrl_reg = readl_relaxed(base + REG_MMU_CTRL_REG);
 	reg->vld_pa_rng = readl_relaxed(base + REG_MMU_VLD_PA_RNG);
 	do {
-		if (!data->plat_data->banks_enable[i])
+		if (!data->plat_data->banks_enable[i] || data->bank[i].is_secure)
 			continue;
 		base = data->bank[i].base;
 		reg->int_control[i] = readl_relaxed(base + REG_MMU_INT_CONTROL0);
-- 
2.25.1

[PATCH 4/4] iommu/mediatek: mt8188: Enable secure bank for MM IOMMU

[Yong Wu]

Enable secure bank for MT8188 VDO IOMMU and VPP IOMMU to support
secure video path (SVP) feature. The last bank is the secure bank.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/iommu/mtk_iommu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 24d7f5138f7b..f17046157c79 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1667,11 +1667,11 @@ static const struct mtk_iommu_plat_data mt8188_data_vdo = {
 	.m4u_plat       = M4U_MT8188,
 	.flags          = HAS_BCLK | HAS_SUB_COMM_3BITS | OUT_ORDER_WR_EN |
 			  WR_THROT_EN | IOVA_34_EN | SHARE_PGTABLE |
-			  PGTABLE_PA_35_EN | MTK_IOMMU_TYPE_MM,
+			  PGTABLE_PA_35_EN | MTK_IOMMU_TYPE_MM | SECURE_BANK_ENABLE,
 	.hw_list        = &m4ulist,
 	.inv_sel_reg    = REG_MMU_INV_SEL_GEN2,
-	.banks_num      = 1,
-	.banks_enable   = {true},
+	.banks_num      = 5,
+	.banks_enable   = {true, false, false, false, true},
 	.iova_region    = mt8192_multi_dom,
 	.iova_region_nr = ARRAY_SIZE(mt8192_multi_dom),
 	.iova_region_larb_msk = mt8188_larb_region_msk,
@@ -1684,11 +1684,11 @@ static const struct mtk_iommu_plat_data mt8188_data_vpp = {
 	.m4u_plat       = M4U_MT8188,
 	.flags          = HAS_BCLK | HAS_SUB_COMM_3BITS | OUT_ORDER_WR_EN |
 			  WR_THROT_EN | IOVA_34_EN | SHARE_PGTABLE |
-			  PGTABLE_PA_35_EN | MTK_IOMMU_TYPE_MM,
+			  PGTABLE_PA_35_EN | MTK_IOMMU_TYPE_MM | SECURE_BANK_ENABLE,
 	.hw_list        = &m4ulist,
 	.inv_sel_reg    = REG_MMU_INV_SEL_GEN2,
-	.banks_num      = 1,
-	.banks_enable   = {true},
+	.banks_num      = 5,
+	.banks_enable   = {true, false, false, false, true},
 	.iova_region    = mt8192_multi_dom,
 	.iova_region_nr = ARRAY_SIZE(mt8192_multi_dom),
 	.iova_region_larb_msk = mt8188_larb_region_msk,
-- 
2.25.1

Re: [PATCH 1/4] iommu/mediatek: Initialise the secure bank

[AngeloGioacchino Del Regno]

Il 11/09/23 03:17, Yong Wu ha scritto:
> The lastest IOMMU always have 5 banks, and we always use the last bank
> (id:4) for the secure memory address translation. This patch add a new
> flag (SECURE_BANK_ENABLE) for this feature.
> 
> For the secure bank, its kernel va "base" is not helpful since the
> secure bank registers has already been protected and can only be accessed
> in the secure world. But we still record its register base, because we need
> use it to determine which IOMMU HW the translation fault happen in the
> secure world.
> 
> Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> ---
>   drivers/iommu/mtk_iommu.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 640275873a27..4a2cffb28c61 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -146,6 +146,7 @@
>   #define TF_PORT_TO_ADDR_MT8173		BIT(18)
>   #define INT_ID_PORT_WIDTH_6		BIT(19)
>   #define CFG_IFA_MASTER_IN_ATF		BIT(20)
> +#define SECURE_BANK_ENABLE		BIT(21)
>   
>   #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)	\
>   				((((pdata)->flags) & (mask)) == (_x))
> @@ -162,6 +163,8 @@
>   #define MTK_IOMMU_GROUP_MAX	8
>   #define MTK_IOMMU_BANK_MAX	5
>   
> +#define MTK_IOMMU_SEC_BANKID	4
> +

Is there any SoC (previous, current or future) that may have more than one
secure context bank?

I'm thinking about implementing this differently...

static const struct mtk_iommu_plat_data mt8188_data_vdo = {
	....
	.flags = ..flags.. | ATF_SECURE_BANKS_ENABLE
	.banks_num = 5,
	.banks_enable = {true, false, false, false, true},
	.banks_secure = {false, false, false, false, true},
	....
}

...this would means that you won't need to specify a static SEC_BANKID, as
you'd get that from banks_secure... so that....

>   enum mtk_iommu_plat {
>   	M4U_MT2712,
>   	M4U_MT6779,
> @@ -240,9 +243,13 @@ struct mtk_iommu_plat_data {
>   };
>   
>   struct mtk_iommu_bank_data {
> -	void __iomem			*base;
> +	union {
> +		void __iomem		*base;
> +		phys_addr_t		sec_bank_base;
> +	};
>   	int				irq;
>   	u8				id;
> +	bool				is_secure;
>   	struct device			*parent_dev;
>   	struct mtk_iommu_data		*parent_data;
>   	spinlock_t			tlb_lock; /* lock for tlb range flush */
> @@ -1309,7 +1316,15 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>   			continue;
>   		bank = &data->bank[i];
>   		bank->id = i;
> -		bank->base = base + i * MTK_IOMMU_BANK_SZ;

....this would become:

bank->is_secure = MTK_IOMMU_HAS_FLAG(data->plat_data, ATF_SECURE_BANKS_ENABLE) &&
		  data->plat_data->banks_secure[i];

if (bank->is_secure)
	bank->sec_bank_base = res->start + i * MTK_IOMMU_BANK_SZ;
else
	bank->base = base + i * MTK_IOMMU_BANK_SZ;

> +		if (MTK_IOMMU_HAS_FLAG(data->plat_data, SECURE_BANK_ENABLE) &&
> +		    bank->id == MTK_IOMMU_SEC_BANKID) {
> +			/* Record the secure bank base to indicate which iommu TF in sec world */
> +			bank->sec_bank_base = res->start + i * MTK_IOMMU_BANK_SZ;
> +			bank->is_secure = true;
> +		} else {
> +			bank->base = base + i * MTK_IOMMU_BANK_SZ;
> +			bank->is_secure = false;
> +		}
>   		bank->m4u_dom = NULL;
>   
>   		bank->irq = platform_get_irq(pdev, i);

What do you think?

Cheers,
Angelo

Re: [PATCH 1/4] iommu/mediatek: Initialise the secure bank

[Yong Wu (吴勇)]

On Mon, 2023-09-11 at 11:22 +0200, AngeloGioacchino Del Regno wrote:
> Il 11/09/23 03:17, Yong Wu ha scritto:
> > The lastest IOMMU always have 5 banks, and we always use the last
> > bank
> > (id:4) for the secure memory address translation. This patch add a
> > new
> > flag (SECURE_BANK_ENABLE) for this feature.
> > 
> > For the secure bank, its kernel va "base" is not helpful since the
> > secure bank registers has already been protected and can only be
> > accessed
> > in the secure world. But we still record its register base, because
> > we need
> > use it to determine which IOMMU HW the translation fault happen in
> > the
> > secure world.
> > 
> > Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> >   drivers/iommu/mtk_iommu.c | 19 +++++++++++++++++--
> >   1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 640275873a27..4a2cffb28c61 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -146,6 +146,7 @@
> >   #define TF_PORT_TO_ADDR_MT8173		BIT(18)
> >   #define INT_ID_PORT_WIDTH_6		BIT(19)
> >   #define CFG_IFA_MASTER_IN_ATF		BIT(20)
> > +#define SECURE_BANK_ENABLE		BIT(21)
> >   
> >   #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)	\
> >   				((((pdata)->flags) & (mask)) == (_x))
> > @@ -162,6 +163,8 @@
> >   #define MTK_IOMMU_GROUP_MAX	8
> >   #define MTK_IOMMU_BANK_MAX	5
> >   
> > +#define MTK_IOMMU_SEC_BANKID	4
> > +
> 
> Is there any SoC (previous, current or future) that may have more
> than one
> secure context bank?

Thanks very much for the below detail suggestion. But No, for MM IOMMU,
The bank4 is mandatory the secure bank, and there is only this one
secure bank, and this is the case for all the current projects, we have
no plan to modify this at the moment. Therefore I think a macro is ok
for it.

Thanks.

> 
> I'm thinking about implementing this differently...
> 
> static const struct mtk_iommu_plat_data mt8188_data_vdo = {
> 	....
> 	.flags = ..flags.. | ATF_SECURE_BANKS_ENABLE
> 	.banks_num = 5,
> 	.banks_enable = {true, false, false, false, true},
> 	.banks_secure = {false, false, false, false, true},
> 	....
> }
> 
> ...this would means that you won't need to specify a static
> SEC_BANKID, as
> you'd get that from banks_secure... so that....
> 
> >   enum mtk_iommu_plat {
> >   	M4U_MT2712,
> >   	M4U_MT6779,
> > @@ -240,9 +243,13 @@ struct mtk_iommu_plat_data {
> >   };
> >   
> >   struct mtk_iommu_bank_data {
> > -	void __iomem			*base;
> > +	union {
> > +		void __iomem		*base;
> > +		phys_addr_t		sec_bank_base;
> > +	};
> >   	int				irq;
> >   	u8				id;
> > +	bool				is_secure;
> >   	struct device			*parent_dev;
> >   	struct mtk_iommu_data		*parent_data;
> >   	spinlock_t			tlb_lock; /* lock for tlb range
> > flush */
> > @@ -1309,7 +1316,15 @@ static int mtk_iommu_probe(struct
> > platform_device *pdev)
> >   			continue;
> >   		bank = &data->bank[i];
> >   		bank->id = i;
> > -		bank->base = base + i * MTK_IOMMU_BANK_SZ;
> 
> ....this would become:
> 
> bank->is_secure = MTK_IOMMU_HAS_FLAG(data->plat_data,
> ATF_SECURE_BANKS_ENABLE) &&
> 		  data->plat_data->banks_secure[i];
> 
> if (bank->is_secure)
> 	bank->sec_bank_base = res->start + i * MTK_IOMMU_BANK_SZ;
> else
> 	bank->base = base + i * MTK_IOMMU_BANK_SZ;
> 
> > +		if (MTK_IOMMU_HAS_FLAG(data->plat_data,
> > SECURE_BANK_ENABLE) &&
> > +		    bank->id == MTK_IOMMU_SEC_BANKID) {
> > +			/* Record the secure bank base to indicate
> > which iommu TF in sec world */
> > +			bank->sec_bank_base = res->start + i *
> > MTK_IOMMU_BANK_SZ;
> > +			bank->is_secure = true;
> > +		} else {
> > +			bank->base = base + i * MTK_IOMMU_BANK_SZ;
> > +			bank->is_secure = false;
> > +		}
> >   		bank->m4u_dom = NULL;
> >   
> >   		bank->irq = platform_get_irq(pdev, i);
> 
> What do you think?
> 
> Cheers,
> Angelo

Re: [PATCH 1/4] iommu/mediatek: Initialise the secure bank

[Alexandre Mergnat]

On 25/09/2023 14:50, Yong Wu (吴勇) wrote:
> On Mon, 2023-09-11 at 11:22 +0200, AngeloGioacchino Del Regno wrote:
>> Il 11/09/23 03:17, Yong Wu ha scritto:
>>> The lastest IOMMU always have 5 banks, and we always use the last
>>> bank
>>> (id:4) for the secure memory address translation. This patch add a
>>> new
>>> flag (SECURE_BANK_ENABLE) for this feature.
>>>
>>> For the secure bank, its kernel va "base" is not helpful since the
>>> secure bank registers has already been protected and can only be
>>> accessed
>>> in the secure world. But we still record its register base, because
>>> we need
>>> use it to determine which IOMMU HW the translation fault happen in
>>> the
>>> secure world.
>>>
>>> Signed-off-by: Anan Sun <anan.sun@mediatek.com>
>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>> ---
>>>    drivers/iommu/mtk_iommu.c | 19 +++++++++++++++++--
>>>    1 file changed, 17 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>> index 640275873a27..4a2cffb28c61 100644
>>> --- a/drivers/iommu/mtk_iommu.c
>>> +++ b/drivers/iommu/mtk_iommu.c
>>> @@ -146,6 +146,7 @@
>>>    #define TF_PORT_TO_ADDR_MT8173		BIT(18)
>>>    #define INT_ID_PORT_WIDTH_6		BIT(19)
>>>    #define CFG_IFA_MASTER_IN_ATF		BIT(20)
>>> +#define SECURE_BANK_ENABLE		BIT(21)
>>>    
>>>    #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)	\
>>>    				((((pdata)->flags) & (mask)) == (_x))
>>> @@ -162,6 +163,8 @@
>>>    #define MTK_IOMMU_GROUP_MAX	8
>>>    #define MTK_IOMMU_BANK_MAX	5
>>>    
>>> +#define MTK_IOMMU_SEC_BANKID	4
>>> +
>>
>> Is there any SoC (previous, current or future) that may have more
>> than one
>> secure context bank?
> 
> Thanks very much for the below detail suggestion. But No, for MM IOMMU,
> The bank4 is mandatory the secure bank, and there is only this one
> secure bank, and this is the case for all the current projects, we have
> no plan to modify this at the moment. Therefore I think a macro is ok
> for it.
> 

Between 2 solutions which have the equivalent complexity (logical & 
readability), I prefer the most generic one (at least for generic 
drivers like this). Nobody is aware about future SoC, even if you know 
what will have the next SoC generation, I'm not sure you can certified 
it will be the same in the next 2, 3, 4,... generations.

I'm convinced it will be easier in the future to maintain the IOMMU code 
if it's flexible.

> Thanks.
> 
>>
>> I'm thinking about implementing this differently...
>>
>> static const struct mtk_iommu_plat_data mt8188_data_vdo = {
>> 	....
>> 	.flags = ..flags.. | ATF_SECURE_BANKS_ENABLE
>> 	.banks_num = 5,
>> 	.banks_enable = {true, false, false, false, true},
>> 	.banks_secure = {false, false, false, false, true},
>> 	....
>> }
>>
>> ...this would means that you won't need to specify a static
>> SEC_BANKID, as
>> you'd get that from banks_secure... so that....
>>
>>>    enum mtk_iommu_plat {
>>>    	M4U_MT2712,
>>>    	M4U_MT6779,
>>> @@ -240,9 +243,13 @@ struct mtk_iommu_plat_data {
>>>    };
>>>    
>>>    struct mtk_iommu_bank_data {
>>> -	void __iomem			*base;
>>> +	union {
>>> +		void __iomem		*base;
>>> +		phys_addr_t		sec_bank_base;
>>> +	};
>>>    	int				irq;
>>>    	u8				id;
>>> +	bool				is_secure;
>>>    	struct device			*parent_dev;
>>>    	struct mtk_iommu_data		*parent_data;
>>>    	spinlock_t			tlb_lock; /* lock for tlb range
>>> flush */
>>> @@ -1309,7 +1316,15 @@ static int mtk_iommu_probe(struct
>>> platform_device *pdev)
>>>    			continue;
>>>    		bank = &data->bank[i];
>>>    		bank->id = i;
>>> -		bank->base = base + i * MTK_IOMMU_BANK_SZ;
>>
>> ....this would become:
>>
>> bank->is_secure = MTK_IOMMU_HAS_FLAG(data->plat_data,
>> ATF_SECURE_BANKS_ENABLE) &&
>> 		  data->plat_data->banks_secure[i];
>>
>> if (bank->is_secure)
>> 	bank->sec_bank_base = res->start + i * MTK_IOMMU_BANK_SZ;
>> else
>> 	bank->base = base + i * MTK_IOMMU_BANK_SZ;
>>
>>> +		if (MTK_IOMMU_HAS_FLAG(data->plat_data,
>>> SECURE_BANK_ENABLE) &&
>>> +		    bank->id == MTK_IOMMU_SEC_BANKID) {
>>> +			/* Record the secure bank base to indicate
>>> which iommu TF in sec world */
>>> +			bank->sec_bank_base = res->start + i *
>>> MTK_IOMMU_BANK_SZ;
>>> +			bank->is_secure = true;
>>> +		} else {
>>> +			bank->base = base + i * MTK_IOMMU_BANK_SZ;
>>> +			bank->is_secure = false;
>>> +		}
>>>    		bank->m4u_dom = NULL;
>>>    
>>>    		bank->irq = platform_get_irq(pdev, i);
>>
>> What do you think?
>>
>> Cheers,
>> Angelo

-- 
Regards,
Alexandre

Re: [PATCH 1/4] iommu/mediatek: Initialise the secure bank

[Yong Wu (吴勇)]

On Mon, 2023-09-25 at 20:01 +0200, Alexandre Mergnat wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  
> 
> On 25/09/2023 14:50, Yong Wu (吴勇) wrote:
> > On Mon, 2023-09-11 at 11:22 +0200, AngeloGioacchino Del Regno
> wrote:
> >> Il 11/09/23 03:17, Yong Wu ha scritto:
> >>> The lastest IOMMU always have 5 banks, and we always use the last
> >>> bank
> >>> (id:4) for the secure memory address translation. This patch add
> a
> >>> new
> >>> flag (SECURE_BANK_ENABLE) for this feature.
> >>>
> >>> For the secure bank, its kernel va "base" is not helpful since
> the
> >>> secure bank registers has already been protected and can only be
> >>> accessed
> >>> in the secure world. But we still record its register base,
> because
> >>> we need
> >>> use it to determine which IOMMU HW the translation fault happen
> in
> >>> the
> >>> secure world.
> >>>
> >>> Signed-off-by: Anan Sun <anan.sun@mediatek.com>
> >>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> >>> ---
> >>>    drivers/iommu/mtk_iommu.c | 19 +++++++++++++++++--
> >>>    1 file changed, 17 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/mtk_iommu.c
> b/drivers/iommu/mtk_iommu.c
> >>> index 640275873a27..4a2cffb28c61 100644
> >>> --- a/drivers/iommu/mtk_iommu.c
> >>> +++ b/drivers/iommu/mtk_iommu.c
> >>> @@ -146,6 +146,7 @@
> >>>    #define TF_PORT_TO_ADDR_MT8173BIT(18)
> >>>    #define INT_ID_PORT_WIDTH_6BIT(19)
> >>>    #define CFG_IFA_MASTER_IN_ATFBIT(20)
> >>> +#define SECURE_BANK_ENABLEBIT(21)
> >>>    
> >>>    #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)\
> >>>    ((((pdata)->flags) & (mask)) == (_x))
> >>> @@ -162,6 +163,8 @@
> >>>    #define MTK_IOMMU_GROUP_MAX8
> >>>    #define MTK_IOMMU_BANK_MAX5
> >>>    
> >>> +#define MTK_IOMMU_SEC_BANKID4
> >>> +
> >>
> >> Is there any SoC (previous, current or future) that may have more
> >> than one
> >> secure context bank?
> > 
> > Thanks very much for the below detail suggestion. But No, for MM
> IOMMU,
> > The bank4 is mandatory the secure bank, and there is only this one
> > secure bank, and this is the case for all the current projects, we
> have
> > no plan to modify this at the moment. Therefore I think a macro is
> ok
> > for it.
> > 
> 
> Between 2 solutions which have the equivalent complexity (logical & 
> readability), I prefer the most generic one (at least for generic 
> drivers like this). Nobody is aware about future SoC, even if you
> know 
> what will have the next SoC generation, I'm not sure you can
> certified 
> it will be the same in the next 2, 3, 4,... generations.

I don't think the 2 solutions is not equivalent. If we add a new
"banks_secure", for readability, we need add it for each current SoC.
This looks more complex. In current version I use a fixed value, which
is simpler, but of course lacks flexibility, which is what you are
worried about.

But we really have no plans to change this. Of course I can't be sure
what will happen in a few years. I think it's not complicated to
modify, let's modify if when necessary?

Thanks.

> 
> I'm convinced it will be easier in the future to maintain the IOMMU
> code 
> if it's flexible.
> 
> > Thanks.
> > 
> >>
> >> I'm thinking about implementing this differently...
> >>
> >> static const struct mtk_iommu_plat_data mt8188_data_vdo = {
> >> ....
> >> .flags = ..flags.. | ATF_SECURE_BANKS_ENABLE
> >> .banks_num = 5,
> >> .banks_enable = {true, false, false, false, true},
> >> .banks_secure = {false, false, false, false, true},
> >> ....
> >> }
> >>
> >> ...this would means that you won't need to specify a static
> >> SEC_BANKID, as
> >> you'd get that from banks_secure... so that....
> >>
> >>>    enum mtk_iommu_plat {
> >>>    M4U_MT2712,
> >>>    M4U_MT6779,
> >>> @@ -240,9 +243,13 @@ struct mtk_iommu_plat_data {
> >>>    };
> >>>    
> >>>    struct mtk_iommu_bank_data {
> >>> -void __iomem*base;
> >>> +union {
> >>> +void __iomem*base;
> >>> +phys_addr_tsec_bank_base;
> >>> +};
> >>>    intirq;
> >>>    u8id;
> >>> +boolis_secure;
> >>>    struct device*parent_dev;
> >>>    struct mtk_iommu_data*parent_data;
> >>>    spinlock_ttlb_lock; /* lock for tlb range
> >>> flush */
> >>> @@ -1309,7 +1316,15 @@ static int mtk_iommu_probe(struct
> >>> platform_device *pdev)
> >>>    continue;
> >>>    bank = &data->bank[i];
> >>>    bank->id = i;
> >>> -bank->base = base + i * MTK_IOMMU_BANK_SZ;
> >>
> >> ....this would become:
> >>
> >> bank->is_secure = MTK_IOMMU_HAS_FLAG(data->plat_data,
> >> ATF_SECURE_BANKS_ENABLE) &&
> >>   data->plat_data->banks_secure[i];
> >>
> >> if (bank->is_secure)
> >> bank->sec_bank_base = res->start + i * MTK_IOMMU_BANK_SZ;
> >> else
> >> bank->base = base + i * MTK_IOMMU_BANK_SZ;
> >>
> >>> +if (MTK_IOMMU_HAS_FLAG(data->plat_data,
> >>> SECURE_BANK_ENABLE) &&
> >>> +    bank->id == MTK_IOMMU_SEC_BANKID) {
> >>> +/* Record the secure bank base to indicate
> >>> which iommu TF in sec world */
> >>> +bank->sec_bank_base = res->start + i *
> >>> MTK_IOMMU_BANK_SZ;
> >>> +bank->is_secure = true;
> >>> +} else {
> >>> +bank->base = base + i * MTK_IOMMU_BANK_SZ;
> >>> +bank->is_secure = false;
> >>> +}
> >>>    bank->m4u_dom = NULL;
> >>>    
> >>>    bank->irq = platform_get_irq(pdev, i);
> >>
> >> What do you think?
> >>
> >> Cheers,
> >> Angelo
> 
> -- 
> Regards,
> Alexandre