[PATCH v3 0/3] MT8188 SMI SUPPORT

[PATCH v3 0/3] MT8188 SMI SUPPORT

[Chengci.Xu]

This patchset adds MT8188 SMI support.

MT8188, similar to mt8195, there are two SMI-common HW, one is for
VDO(video output), the other is for VPP(video processing pipe). They
connect with different SMI-larbs, then some setting(bus_sel) is
different.

The connection between SMI and MM IOMMU could be something like this:

  IOMMU(VDO)           IOMMU(VPP)
     |                    |
SMI_COMMON_VDO       SMI_COMMON_VPP
----------------     ----------------
  |     |   ...       |     |    ...
larb0 larb2 ...     larb1 larb3  ...

Another change is that the register about enable/disable iommu is in
security world. We add some SMC command to set it.

changes since v3:
  - base on tag: next-20220726.
  - No code change just remove the change-id in patch [2/3].

changes since v2:
  - base on tag: next-20220722.
  - Move some included header to the source file that use them.

changes since v1:
  - base on tag: next-20220720.
  - adds MT8188 SMI support.

Chengci.Xu (3):
  dt-bindings: memory: mediatek: Add mt8188 smi binding
  memory: mtk-smi: Add enable IOMMU SMC command for MM master
  memory: mtk-smi: mt8188: Add SMI Support

 .../mediatek,smi-common.yaml                  |  4 +-
 .../memory-controllers/mediatek,smi-larb.yaml |  3 +
 drivers/memory/mtk-smi.c                      | 82 +++++++++++++++++++
 include/linux/soc/mediatek/mtk_sip_svc.h      |  3 +
 include/soc/mediatek/smi.h                    |  7 ++
 5 files changed, 98 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v3 1/3] dt-bindings: memory: mediatek: Add mt8188 smi binding

[Chengci.Xu]

Add mt8188 smi supporting in the bindings.

In mt8188, there are two smi-common HW, one is for vdo(video output),
the other is for vpp(video processing pipe). They connect with different
smi-larbs, then some setting(bus_sel) is different. Differentiate them
with the compatible string.

Something like this:

   IOMMU(VDO)          IOMMU(VPP)
       |                   |
SMI_COMMON_VDO       SMI_COMMON_VPP
----------------     ----------------
   |     |   ...       |     |    ...
 larb0 larb2 ...     larb1 larb3  ...

Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
---
 .../bindings/memory-controllers/mediatek,smi-common.yaml      | 4 +++-
 .../bindings/memory-controllers/mediatek,smi-larb.yaml        | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
index 71bc5cefb49c..70bba66c7551 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
@@ -16,7 +16,7 @@ description: |
   MediaTek SMI have two generations of HW architecture, here is the list
   which generation the SoCs use:
   generation 1: mt2701 and mt7623.
-  generation 2: mt2712, mt6779, mt8167, mt8173, mt8183, mt8186, mt8192 and mt8195.
+  generation 2: mt2712, mt6779, mt8167, mt8173, mt8183, mt8186, mt8188, mt8192 and mt8195.
 
   There's slight differences between the two SMI, for generation 2, the
   register which control the iommu port is at each larb's register base. But
@@ -37,6 +37,8 @@ properties:
           - mediatek,mt8173-smi-common
           - mediatek,mt8183-smi-common
           - mediatek,mt8186-smi-common
+          - mediatek,mt8188-smi-common-vdo
+          - mediatek,mt8188-smi-common-vpp
           - mediatek,mt8192-smi-common
           - mediatek,mt8195-smi-common-vdo
           - mediatek,mt8195-smi-common-vpp
diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
index 59dcd163668f..5f4ac3609887 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
@@ -25,6 +25,7 @@ properties:
           - mediatek,mt8173-smi-larb
           - mediatek,mt8183-smi-larb
           - mediatek,mt8186-smi-larb
+          - mediatek,mt8188-smi-larb
           - mediatek,mt8192-smi-larb
           - mediatek,mt8195-smi-larb
 
@@ -78,6 +79,7 @@ allOf:
           enum:
             - mediatek,mt8183-smi-larb
             - mediatek,mt8186-smi-larb
+            - mediatek,mt8188-smi-larb
             - mediatek,mt8195-smi-larb
 
     then:
@@ -111,6 +113,7 @@ allOf:
               - mediatek,mt2712-smi-larb
               - mediatek,mt6779-smi-larb
               - mediatek,mt8186-smi-larb
+              - mediatek,mt8188-smi-larb
               - mediatek,mt8192-smi-larb
               - mediatek,mt8195-smi-larb
 
-- 
2.25.1

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

[Chengci.Xu]

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)
 #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);
+	}
 }
 
 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,
+};
+
 #if IS_ENABLED(CONFIG_MTK_SMI)
 
 #define MTK_SMI_MMU_EN(port)	BIT(port)
-- 
2.25.1

[PATCH v3 3/3] memory: mtk-smi: mt8188: Add SMI Support

[Chengci.Xu]

Add mt8188 smi common & larb support

Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
---
 drivers/memory/mtk-smi.c | 71 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index 41ce66c39123..5ac2547b1480 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -272,6 +272,55 @@ static void mtk_smi_larb_config_port_gen2_general(struct device *dev)
 	}
 }
 
+static const u8 mtk_smi_larb_mt8188_ostd[][SMI_LARB_PORT_NR_MAX] = {
+	[0] = {0x02, 0x18, 0x22, 0x22, 0x01, 0x02, 0x0a,},
+	[1] = {0x12, 0x02, 0x14, 0x14, 0x01, 0x18, 0x0a,},
+	[2] = {0x12, 0x12, 0x12, 0x12, 0x0a,},
+	[3] = {0x12, 0x12, 0x12, 0x12, 0x28, 0x28, 0x0a,},
+	[4] = {0x06, 0x01, 0x17, 0x06, 0x0a, 0x07, 0x07,},
+	[5] = {0x02, 0x01, 0x04, 0x02, 0x06, 0x01, 0x06, 0x0a,},
+	[6] = {0x06, 0x01, 0x06, 0x0a,},
+	[7] = {0x0c, 0x0c, 0x12,},
+	[8] = {0x0c, 0x01, 0x0a, 0x05, 0x02, 0x03, 0x01, 0x01, 0x14, 0x14,
+	       0x0a, 0x14, 0x1e, 0x01, 0x0c, 0x0a, 0x05, 0x02, 0x02, 0x05,
+	       0x03, 0x01, 0x1e, 0x01, 0x05,},
+	[9] = {0x1e, 0x01, 0x0a, 0x0a, 0x01, 0x01, 0x03, 0x1e, 0x1e, 0x10,
+	       0x07, 0x01, 0x0a, 0x06, 0x03, 0x03, 0x0e, 0x01, 0x04, 0x28,},
+	[10] = {0x03, 0x20, 0x01, 0x20, 0x01, 0x01, 0x14, 0x0a, 0x0a, 0x0c,
+		0x0a, 0x05, 0x02, 0x03, 0x02, 0x14, 0x0a, 0x0a, 0x14, 0x14,
+		0x14, 0x01, 0x01, 0x14, 0x1e, 0x01, 0x05, 0x03, 0x02, 0x28,},
+	[11] = {0x03, 0x20, 0x01, 0x20, 0x01, 0x01, 0x14, 0x0a, 0x0a, 0x0c,
+		0x0a, 0x05, 0x02, 0x03, 0x02, 0x14, 0x0a, 0x0a, 0x14, 0x14,
+		0x14, 0x01, 0x01, 0x14, 0x1e, 0x01, 0x05, 0x03, 0x02, 0x28,},
+	[12] = {0x03, 0x20, 0x01, 0x20, 0x01, 0x01, 0x14, 0x0a, 0x0a, 0x0c,
+		0x0a, 0x05, 0x02, 0x03, 0x02, 0x14, 0x0a, 0x0a, 0x14, 0x14,
+		0x14, 0x01, 0x01, 0x14, 0x1e, 0x01, 0x05, 0x03, 0x02, 0x28,},
+	[13] = {0x07, 0x02, 0x04, 0x02, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05,
+		0x07, 0x02, 0x04, 0x02, 0x05, 0x05,},
+	[14] = {0x02, 0x02, 0x0c, 0x0c, 0x0c, 0x0c, 0x01, 0x01, 0x02, 0x02,
+		0x02, 0x02, 0x0c, 0x0c, 0x02, 0x02, 0x02, 0x02, 0x02, 0x02,
+		0x02, 0x02, 0x01, 0x01,},
+	[15] = {0x0c, 0x0c, 0x02, 0x02, 0x02, 0x02, 0x01, 0x01, 0x0c, 0x0c,
+		0x02, 0x02, 0x02, 0x02, 0x02, 0x02, 0x02, 0x02, 0x01, 0x02,
+		0x0c, 0x01, 0x01,},
+	[16] = {0x28, 0x28, 0x03, 0x01, 0x01, 0x03, 0x14, 0x14, 0x0a, 0x0d,
+		0x03, 0x05, 0x0e, 0x01, 0x01, 0x05, 0x06, 0x0d, 0x01,},
+	[17] = {0x28, 0x02, 0x02, 0x12, 0x02, 0x12, 0x10, 0x02, 0x02, 0x0a,
+		0x12, 0x02, 0x02, 0x0a, 0x16, 0x02, 0x04,},
+	[18] = {0x28, 0x02, 0x02, 0x12, 0x02, 0x12, 0x10, 0x02, 0x02, 0x0a,
+		0x12, 0x02, 0x02, 0x0a, 0x16, 0x02, 0x04,},
+	[19] = {0x1a, 0x0e, 0x0a, 0x0a, 0x0c, 0x0e, 0x10,},
+	[20] = {0x1a, 0x0e, 0x0a, 0x0a, 0x0c, 0x0e, 0x10,},
+	[21] = {0x01, 0x04, 0x01, 0x01, 0x01, 0x01, 0x01, 0x04, 0x04, 0x01,
+		0x01, 0x01, 0x04, 0x0a, 0x06, 0x01, 0x01, 0x01, 0x0a, 0x06,
+		0x01, 0x01, 0x05, 0x03, 0x03, 0x04, 0x01,},
+	[22] = {0x28, 0x19, 0x0c, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x04,
+		0x01,},
+	[23] = {0x01, 0x01, 0x04, 0x01, 0x01, 0x01, 0x18, 0x01, 0x01,},
+	[24] = {0x12, 0x06, 0x12, 0x06,},
+	[25] = {0x01},
+};
+
 static const u8 mtk_smi_larb_mt8195_ostd[][SMI_LARB_PORT_NR_MAX] = {
 	[0] = {0x0a, 0xc, 0x22, 0x22, 0x01, 0x0a,}, /* larb0 */
 	[1] = {0x0a, 0xc, 0x22, 0x22, 0x01, 0x0a,}, /* larb1 */
@@ -358,6 +407,13 @@ static const struct mtk_smi_larb_gen mtk_smi_larb_mt8186 = {
 	.flags_general	            = MTK_SMI_FLAG_SLEEP_CTL,
 };
 
+static const struct mtk_smi_larb_gen mtk_smi_larb_mt8188 = {
+	.config_port                = mtk_smi_larb_config_port_gen2_general,
+	.flags_general	            = MTK_SMI_FLAG_THRT_UPDATE | MTK_SMI_FLAG_SW_FLAG |
+				      MTK_SMI_FLAG_SLEEP_CTL | MTK_SMI_FLAG_SEC_REG,
+	.ostd		            = mtk_smi_larb_mt8188_ostd,
+};
+
 static const struct mtk_smi_larb_gen mtk_smi_larb_mt8192 = {
 	.config_port                = mtk_smi_larb_config_port_gen2_general,
 };
@@ -378,6 +434,7 @@ static const struct of_device_id mtk_smi_larb_of_ids[] = {
 	{.compatible = "mediatek,mt8173-smi-larb", .data = &mtk_smi_larb_mt8173},
 	{.compatible = "mediatek,mt8183-smi-larb", .data = &mtk_smi_larb_mt8183},
 	{.compatible = "mediatek,mt8186-smi-larb", .data = &mtk_smi_larb_mt8186},
+	{.compatible = "mediatek,mt8188-smi-larb", .data = &mtk_smi_larb_mt8188},
 	{.compatible = "mediatek,mt8192-smi-larb", .data = &mtk_smi_larb_mt8192},
 	{.compatible = "mediatek,mt8195-smi-larb", .data = &mtk_smi_larb_mt8195},
 	{}
@@ -608,6 +665,18 @@ static const struct mtk_smi_common_plat mtk_smi_common_mt8186 = {
 	.bus_sel  = F_MMU1_LARB(1) | F_MMU1_LARB(4) | F_MMU1_LARB(7),
 };
 
+static const struct mtk_smi_common_plat mtk_smi_common_mt8188_vdo = {
+	.type     = MTK_SMI_GEN2,
+	.bus_sel  = F_MMU1_LARB(1) | F_MMU1_LARB(5) | F_MMU1_LARB(7),
+	.init     = mtk_smi_common_mt8195_init,
+};
+
+static const struct mtk_smi_common_plat mtk_smi_common_mt8188_vpp = {
+	.type     = MTK_SMI_GEN2,
+	.bus_sel  = F_MMU1_LARB(1) | F_MMU1_LARB(2) | F_MMU1_LARB(7),
+	.init     = mtk_smi_common_mt8195_init,
+};
+
 static const struct mtk_smi_common_plat mtk_smi_common_mt8192 = {
 	.type     = MTK_SMI_GEN2,
 	.has_gals = true,
@@ -644,6 +713,8 @@ static const struct of_device_id mtk_smi_common_of_ids[] = {
 	{.compatible = "mediatek,mt8173-smi-common", .data = &mtk_smi_common_gen2},
 	{.compatible = "mediatek,mt8183-smi-common", .data = &mtk_smi_common_mt8183},
 	{.compatible = "mediatek,mt8186-smi-common", .data = &mtk_smi_common_mt8186},
+	{.compatible = "mediatek,mt8188-smi-common-vdo", .data = &mtk_smi_common_mt8188_vdo},
+	{.compatible = "mediatek,mt8188-smi-common-vpp", .data = &mtk_smi_common_mt8188_vpp},
 	{.compatible = "mediatek,mt8192-smi-common", .data = &mtk_smi_common_mt8192},
 	{.compatible = "mediatek,mt8195-smi-common-vdo", .data = &mtk_smi_common_mt8195_vdo},
 	{.compatible = "mediatek,mt8195-smi-common-vpp", .data = &mtk_smi_common_mt8195_vpp},
-- 
2.25.1

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

[Yong Wu]

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)

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

[Yong Wu]

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)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/3] memory: mtk-smi: mt8188: Add SMI Support

[Yong Wu]

On Wed, 2022-07-27 at 18:45 +0800, Chengci.Xu wrote:
> Add mt8188 smi common & larb support
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>

Reviewed-by: Yong Wu <yong.wu@mediatek.com>

> ---
>  drivers/memory/mtk-smi.c | 71
> ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)

Re: [PATCH v3 3/3] memory: mtk-smi: mt8188: Add SMI Support

[Yong Wu]

On Wed, 2022-07-27 at 18:45 +0800, Chengci.Xu wrote:
> Add mt8188 smi common & larb support
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>

Reviewed-by: Yong Wu <yong.wu@mediatek.com>

> ---
>  drivers/memory/mtk-smi.c | 71
> ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] dt-bindings: memory: mediatek: Add mt8188 smi binding

[AngeloGioacchino Del Regno]

Il 27/07/22 12:45, Chengci.Xu ha scritto:
> Add mt8188 smi supporting in the bindings.
> 
> In mt8188, there are two smi-common HW, one is for vdo(video output),
> the other is for vpp(video processing pipe). They connect with different
> smi-larbs, then some setting(bus_sel) is different. Differentiate them
> with the compatible string.
> 
> Something like this:
> 
>     IOMMU(VDO)          IOMMU(VPP)
>         |                   |
> SMI_COMMON_VDO       SMI_COMMON_VPP
> ----------------     ----------------
>     |     |   ...       |     |    ...
>   larb0 larb2 ...     larb1 larb3  ...
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>

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

Re: [PATCH v3 1/3] dt-bindings: memory: mediatek: Add mt8188 smi binding

[AngeloGioacchino Del Regno]

Il 27/07/22 12:45, Chengci.Xu ha scritto:
> Add mt8188 smi supporting in the bindings.
> 
> In mt8188, there are two smi-common HW, one is for vdo(video output),
> the other is for vpp(video processing pipe). They connect with different
> smi-larbs, then some setting(bus_sel) is different. Differentiate them
> with the compatible string.
> 
> Something like this:
> 
>     IOMMU(VDO)          IOMMU(VPP)
>         |                   |
> SMI_COMMON_VDO       SMI_COMMON_VPP
> ----------------     ----------------
>     |     |   ...       |     |    ...
>   larb0 larb2 ...     larb1 larb3  ...
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/3] memory: mtk-smi: mt8188: Add SMI Support

[AngeloGioacchino Del Regno]

Il 27/07/22 12:45, Chengci.Xu ha scritto:
> Add mt8188 smi common & larb support
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
> Reviewed-by: Yong Wu <yong.wu@mediatek.com>

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

Re: [PATCH v3 3/3] memory: mtk-smi: mt8188: Add SMI Support

[AngeloGioacchino Del Regno]

Il 27/07/22 12:45, Chengci.Xu ha scritto:
> Add mt8188 smi common & larb support
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
> Reviewed-by: Yong Wu <yong.wu@mediatek.com>

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[AngeloGioacchino Del Regno]

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@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)
>   #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.

> +	}
>   }
>   
>   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?

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

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

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

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

[AngeloGioacchino Del Regno]

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@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)
>   #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.

> +	}
>   }
>   
>   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?

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

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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@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

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)
> 
> 

Re: [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@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

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)
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@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)
> >   #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)
> 
> 

Re: [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@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)
> >   #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)
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel