[PATCH v4] Add MediaTek MT6779 devapc driver

[PATCH v4] Add MediaTek MT6779 devapc driver

[Neal Liu]

These patch series introduce a MediaTek MT6779 devapc driver.

MediaTek bus fabric provides TrustZone security support and data protection to prevent slaves from being accessed by unexpected masters.
The security violation is logged and sent to the processor for further analysis or countermeasures.

Any occurrence of security violation would raise an interrupt, and it will be handled by mtk-devapc driver.
The violation information is printed in order to find the murderer.

changes since v3:
- revise violation handling flow to make it more easily to understand
  hardware behavior.
- add more comments to understand how hardware works.

changes since v2:
- pass platform info through DT data.
- remove unnecessary function.
- remove slave_type because it always equals to 1 in current support SoC.
- use vio_idx_num instread of list all devices' index.
- add more comments to describe hardware behavior.

changes since v1:
- move SoC specific part to DT data.
- remove unnecessary boundary check.
- remove unnecessary data type declaration.
- use read_poll_timeout() instread of for loop polling.
- revise coding style elegantly.


*** BLURB HERE ***

Neal Liu (2):
  dt-bindings: devapc: add bindings for mtk-devapc
  soc: mediatek: add mtk-devapc driver

 .../bindings/soc/mediatek/devapc.yaml         |  58 ++++
 drivers/soc/mediatek/Kconfig                  |   9 +
 drivers/soc/mediatek/Makefile                 |   1 +
 drivers/soc/mediatek/mtk-devapc.c             | 323 ++++++++++++++++++
 drivers/soc/mediatek/mtk-devapc.h             |  54 +++
 5 files changed, 445 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
 create mode 100644 drivers/soc/mediatek/mtk-devapc.c
 create mode 100644 drivers/soc/mediatek/mtk-devapc.h

-- 
2.18.0

[PATCH v4 1/2] dt-bindings: devapc: add bindings for mtk-devapc

[Neal Liu]

Add bindings for mtk-devapc.

Signed-off-by: Neal Liu <neal.liu@mediatek.com>
---
 .../devicetree/bindings/soc/mediatek/devapc.yaml   |   58 ++++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc.yaml

diff --git a/Documentation/devicetree/bindings/soc/mediatek/devapc.yaml b/Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
new file mode 100644
index 0000000..6c763f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# # Copyright 2020 MediaTek Inc.
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/soc/mediatek/devapc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: MediaTek Device Access Permission Control driver
+
+description: |
+  MediaTek bus fabric provides TrustZone security support and data
+  protection to prevent slaves from being accessed by unexpected masters.
+  The security violation is logged and sent to the processor for further
+  analysis and countermeasures.
+
+maintainers:
+  - Neal Liu <neal.liu@mediatek.com>
+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt6779-devapc
+
+  reg:
+    description: The base address of devapc register bank
+    maxItems: 1
+
+  interrupts:
+    description: A single interrupt specifier
+    maxItems: 1
+
+  clocks:
+    description: Contains module clock source and clock names
+    maxItems: 1
+
+  clock-names:
+    description: Names of the clocks list in clocks property
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/mt6779-clk.h>
+
+    devapc: devapc@10207000 {
+      compatible = "mediatek,mt6779-devapc";
+      reg = <0x10207000 0x1000>;
+      interrupts = <GIC_SPI 168 IRQ_TYPE_LEVEL_LOW>;
+      clocks = <&infracfg_ao CLK_INFRA_DEVICE_APC>;
+      clock-names = "devapc-infra-clock";
+    };
-- 
1.7.9.5

[PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

MediaTek bus fabric provides TrustZone security support and data
protection to prevent slaves from being accessed by unexpected
masters.
The security violation is logged and sent to the processor for
further analysis or countermeasures.

Any occurrence of security violation would raise an interrupt, and
it will be handled by mtk-devapc driver. The violation
information is printed in order to find the murderer.

Signed-off-by: Neal Liu <neal.liu@mediatek.com>
---
 drivers/soc/mediatek/Kconfig      |    9 ++
 drivers/soc/mediatek/Makefile     |    1 +
 drivers/soc/mediatek/mtk-devapc.c |  323 +++++++++++++++++++++++++++++++++++++
 drivers/soc/mediatek/mtk-devapc.h |   54 +++++++
 4 files changed, 387 insertions(+)
 create mode 100644 drivers/soc/mediatek/mtk-devapc.c
 create mode 100644 drivers/soc/mediatek/mtk-devapc.h

diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
index 59a56cd..1177c98 100644
--- a/drivers/soc/mediatek/Kconfig
+++ b/drivers/soc/mediatek/Kconfig
@@ -17,6 +17,15 @@ config MTK_CMDQ
 	  time limitation, such as updating display configuration during the
 	  vblank.
 
+config MTK_DEVAPC
+	tristate "Mediatek Device APC Support"
+	help
+	  Say yes here to enable support for Mediatek Device APC driver.
+	  This driver is mainly used to handle the violation which catches
+	  unexpected transaction.
+	  The violation information is logged for further analysis or
+	  countermeasures.
+
 config MTK_INFRACFG
 	bool "MediaTek INFRACFG Support"
 	select REGMAP
diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index 01f9f87..abfd4ba 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
+obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o
 obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
 obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
 obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
diff --git a/drivers/soc/mediatek/mtk-devapc.c b/drivers/soc/mediatek/mtk-devapc.c
new file mode 100644
index 0000000..db751d0
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-devapc.c
@@ -0,0 +1,323 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 MediaTek Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include "mtk-devapc.h"
+
+static int get_shift_group(struct mtk_devapc_context *ctx)
+{
+	u32 vio_shift_sta;
+	void __iomem *reg;
+
+	reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
+	vio_shift_sta = readl(reg);
+
+	if (vio_shift_sta)
+		return __ffs(vio_shift_sta);
+
+	return -EIO;
+}
+
+static void clear_vio_status(struct mtk_devapc_context *ctx, u32 module)
+{
+	void __iomem *reg;
+
+	reg = ctx->devapc_pd_base + ctx->offset->vio_sta;
+	reg += 0x4 * VIO_MOD_TO_REG_IND(module);
+
+	writel(0x1 << VIO_MOD_TO_REG_OFF(module), reg);
+}
+
+static void mask_module_irq(struct mtk_devapc_context *ctx, u32 module,
+			    bool mask)
+{
+	void __iomem *reg;
+	u32 value;
+
+	reg = ctx->devapc_pd_base + ctx->offset->vio_mask;
+	reg += 0x4 * VIO_MOD_TO_REG_IND(module);
+
+	value = readl(reg);
+	if (mask)
+		value |= (0x1 << VIO_MOD_TO_REG_OFF(module));
+	else
+		value &= ~(0x1 << VIO_MOD_TO_REG_OFF(module));
+
+	writel(value, reg);
+}
+
+#define PHY_DEVAPC_TIMEOUT	0x10000
+
+/*
+ * sync_vio_dbg - do "shift" mechansim" to get full violation information.
+ *                shift mechanism is depends on devapc hardware design.
+ *                Mediatek devapc set multiple slaves as a group. When violation
+ *                is triggered, violation info is kept inside devapc hardware.
+ *                Driver should do shift mechansim to "shift" full violation
+ *                info to VIO_DBGs registers.
+ *
+ */
+static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
+{
+	void __iomem *pd_vio_shift_sta_reg;
+	void __iomem *pd_vio_shift_sel_reg;
+	void __iomem *pd_vio_shift_con_reg;
+	int ret;
+	u32 val;
+
+	pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
+	pd_vio_shift_sel_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sel;
+	pd_vio_shift_con_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_con;
+
+	/* Enable shift mechansim */
+	writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
+	writel(0x1, pd_vio_shift_con_reg);
+
+	ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
+				 PHY_DEVAPC_TIMEOUT);
+	if (ret)
+		dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
+
+	/* Disable shift mechanism */
+	writel(0x0, pd_vio_shift_con_reg);
+	writel(0x0, pd_vio_shift_sel_reg);
+	writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
+
+	return ret;
+}
+
+static void devapc_vio_info_print(struct mtk_devapc_context *ctx)
+{
+	struct mtk_devapc_vio_info *vio_info = ctx->vio_info;
+
+	/* Print violation information */
+	if (vio_info->write)
+		dev_info(ctx->dev, "Write Violation\n");
+	else if (vio_info->read)
+		dev_info(ctx->dev, "Read Violation\n");
+
+	dev_info(ctx->dev, "Vio Addr:0x%x, High:0x%x, Bus ID:0x%x, Dom ID:%x\n",
+		 vio_info->vio_addr, vio_info->vio_addr_high,
+		 vio_info->master_id, vio_info->domain_id);
+}
+
+/*
+ * devapc_extract_vio_dbg - extract full violation information after doing
+ *                          shift mechanism.
+ */
+static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
+{
+	const struct mtk_devapc_vio_dbgs *vio_dbgs;
+	struct mtk_devapc_vio_info *vio_info;
+	void __iomem *vio_dbg0_reg;
+	void __iomem *vio_dbg1_reg;
+	u32 dbg0;
+
+	vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
+	vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
+
+	vio_dbgs = ctx->vio_dbgs;
+	vio_info = ctx->vio_info;
+
+	/* Starts to extract violation information */
+	dbg0 = readl(vio_dbg0_reg);
+	vio_info->vio_addr = readl(vio_dbg1_reg);
+
+	vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
+			      vio_dbgs->mstid.start;
+	vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
+			      vio_dbgs->dmnid.start;
+	vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
+			    vio_dbgs->vio_w.start) == 1;
+	vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
+			  vio_dbgs->vio_r.start) == 1;
+	vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
+				  vio_dbgs->addr_h.start;
+
+	devapc_vio_info_print(ctx);
+}
+
+/*
+ * devapc_dump_vio_dbg - shift and dump the violation debug information.
+ */
+static void devapc_dump_vio_dbg(struct mtk_devapc_context *ctx)
+{
+	int shift_bit;
+
+	/*
+	 * All groups vio_idx belongs to is determined by hardware,
+	 * and each group can only maintain one violation info at a time.
+	 *
+	 * Scan all groups which vio_idx has violation, dump it for
+	 * further analysis.
+	 */
+	while (1) {
+		shift_bit = get_shift_group(ctx);
+
+		if (shift_bit >= 0 && shift_bit <= 31) {
+			if (!sync_vio_dbg(ctx, shift_bit))
+				devapc_extract_vio_dbg(ctx);
+
+			continue;
+		}
+
+		break;
+	}
+}
+
+/*
+ * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
+ *                        violation information including which master violates
+ *                        access slave.
+ */
+static irqreturn_t devapc_violation_irq(int irq_number,
+					struct mtk_devapc_context *ctx)
+{
+	u32 vio_idx;
+
+	/*
+	 * Mask slave's irq before clearing vio status.
+	 * Must do it to avoid nested interrupt and prevent
+	 * unexpected behavior.
+	 */
+	for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
+		mask_module_irq(ctx, vio_idx, true);
+
+	devapc_dump_vio_dbg(ctx);
+
+	/*
+	 * Ensure that violation info are written
+	 * before further operations
+	 */
+	smp_mb();
+
+	for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
+		clear_vio_status(ctx, vio_idx);
+		mask_module_irq(ctx, vio_idx, false);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * start_devapc - unmask slave's irq to start receiving devapc violation.
+ */
+static void start_devapc(struct mtk_devapc_context *ctx)
+{
+	u32 vio_idx;
+
+	for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
+		mask_module_irq(ctx, vio_idx, false);
+}
+
+static const struct mtk_devapc_pd_offset mt6779_pd_offset = {
+	.vio_mask = 0x0,
+	.vio_sta = 0x400,
+	.vio_dbg0 = 0x900,
+	.vio_dbg1 = 0x904,
+	.apc_con = 0xF00,
+	.vio_shift_sta = 0xF10,
+	.vio_shift_sel = 0xF14,
+	.vio_shift_con = 0xF20,
+};
+
+static const struct mtk_devapc_vio_dbgs mt6779_vio_dbgs = {
+	.mstid =  {0x0000FFFF, 0x0},
+	.dmnid =  {0x003F0000, 0x10},
+	.vio_w =  {0x00400000, 0x16},
+	.vio_r =  {0x00800000, 0x17},
+	.addr_h = {0x0F000000, 0x18},
+};
+
+static const struct mtk_devapc_context devapc_mt6779 = {
+	.vio_idx_num = 510,
+	.offset = &mt6779_pd_offset,
+	.vio_dbgs = &mt6779_vio_dbgs,
+};
+
+static const struct of_device_id mtk_devapc_dt_match[] = {
+	{
+		.compatible = "mediatek,mt6779-devapc",
+		.data = &devapc_mt6779,
+	}, {
+	},
+};
+
+static int mtk_devapc_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct mtk_devapc_context *ctx;
+	struct clk *devapc_infra_clk;
+	u32 devapc_irq;
+	int ret;
+
+	if (IS_ERR(node))
+		return -ENODEV;
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx = (struct mtk_devapc_context *)of_device_get_match_data(&pdev->dev);
+	ctx->dev = &pdev->dev;
+
+	ctx->vio_info = devm_kzalloc(&pdev->dev,
+				     sizeof(struct mtk_devapc_vio_info),
+				     GFP_KERNEL);
+	if (!ctx->vio_info)
+		return -ENOMEM;
+
+	ctx->devapc_pd_base = of_iomap(node, 0);
+	if (!ctx->devapc_pd_base)
+		return -EINVAL;
+
+	devapc_irq = irq_of_parse_and_map(node, 0);
+	if (!devapc_irq)
+		return -EINVAL;
+
+	devapc_infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
+	if (IS_ERR(devapc_infra_clk))
+		return -EINVAL;
+
+	if (clk_prepare_enable(devapc_infra_clk))
+		return -EINVAL;
+
+	ret = devm_request_irq(&pdev->dev, devapc_irq,
+			       (irq_handler_t)devapc_violation_irq,
+			       IRQF_TRIGGER_NONE, "devapc", ctx);
+	if (ret)
+		return ret;
+
+	start_devapc(ctx);
+
+	return 0;
+}
+
+static int mtk_devapc_remove(struct platform_device *dev)
+{
+	return 0;
+}
+
+static struct platform_driver mtk_devapc_driver = {
+	.probe = mtk_devapc_probe,
+	.remove = mtk_devapc_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = mtk_devapc_dt_match,
+	},
+};
+
+module_platform_driver(mtk_devapc_driver);
+
+MODULE_DESCRIPTION("Mediatek Device APC Driver");
+MODULE_AUTHOR("Neal Liu <neal.liu@mediatek.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/soc/mediatek/mtk-devapc.h b/drivers/soc/mediatek/mtk-devapc.h
new file mode 100644
index 0000000..7bd7e66
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-devapc.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020 MediaTek Inc.
+ */
+
+#ifndef __MTK_DEVAPC_H__
+#define __MTK_DEVAPC_H__
+
+#define VIO_MOD_TO_REG_IND(m)	((m) / 32)
+#define VIO_MOD_TO_REG_OFF(m)	((m) % 32)
+
+struct mtk_devapc_pd_offset {
+	u32 vio_mask;
+	u32 vio_sta;
+	u32 vio_dbg0;
+	u32 vio_dbg1;
+	u32 apc_con;
+	u32 vio_shift_sta;
+	u32 vio_shift_sel;
+	u32 vio_shift_con;
+};
+
+struct mtk_devapc_vio_dbgs_desc {
+	u32 mask;
+	u32 start;
+};
+
+struct mtk_devapc_vio_dbgs {
+	struct mtk_devapc_vio_dbgs_desc mstid;
+	struct mtk_devapc_vio_dbgs_desc dmnid;
+	struct mtk_devapc_vio_dbgs_desc vio_w;
+	struct mtk_devapc_vio_dbgs_desc vio_r;
+	struct mtk_devapc_vio_dbgs_desc addr_h;
+};
+
+struct mtk_devapc_vio_info {
+	bool read;
+	bool write;
+	u32 vio_addr;
+	u32 vio_addr_high;
+	u32 master_id;
+	u32 domain_id;
+};
+
+struct mtk_devapc_context {
+	struct device *dev;
+	u32 vio_idx_num;
+	void __iomem *devapc_pd_base;
+	struct mtk_devapc_vio_info *vio_info;
+	const struct mtk_devapc_pd_offset *offset;
+	const struct mtk_devapc_vio_dbgs *vio_dbgs;
+};
+
+#endif /* __MTK_DEVAPC_H__ */
-- 
1.7.9.5

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Hi, Neal:

Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> ---

[snip]

> +
> +static int get_shift_group(struct mtk_devapc_context *ctx)
> +{
> +       u32 vio_shift_sta;
> +       void __iomem *reg;
> +
> +       reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> +       vio_shift_sta = readl(reg);
> +
> +       if (vio_shift_sta)
> +               return __ffs(vio_shift_sta);
> +
> +       return -EIO;
> +}

get_shift_group() is a small function, I would like to merge this
function into sync_vio_dbg() to make code more simple.

> +

[snip]

> +
> +#define PHY_DEVAPC_TIMEOUT     0x10000
> +
> +/*
> + * sync_vio_dbg - do "shift" mechansim" to get full violation information.
> + *                shift mechanism is depends on devapc hardware design.
> + *                Mediatek devapc set multiple slaves as a group. When violation
> + *                is triggered, violation info is kept inside devapc hardware.
> + *                Driver should do shift mechansim to "shift" full violation
> + *                info to VIO_DBGs registers.
> + *
> + */
> +static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
> +{
> +       void __iomem *pd_vio_shift_sta_reg;
> +       void __iomem *pd_vio_shift_sel_reg;
> +       void __iomem *pd_vio_shift_con_reg;
> +       int ret;
> +       u32 val;
> +
> +       pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> +       pd_vio_shift_sel_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sel;
> +       pd_vio_shift_con_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_con;
> +
> +       /* Enable shift mechansim */
> +       writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
> +       writel(0x1, pd_vio_shift_con_reg);
> +
> +       ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> +                                PHY_DEVAPC_TIMEOUT);
> +       if (ret)
> +               dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
> +
> +       /* Disable shift mechanism */
> +       writel(0x0, pd_vio_shift_con_reg);
> +       writel(0x0, pd_vio_shift_sel_reg);
> +       writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
> +
> +       return ret;
> +}
> +

[snip]

> +
> +/*
> + * devapc_extract_vio_dbg - extract full violation information after doing
> + *                          shift mechanism.
> + */
> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> +{
> +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> +       struct mtk_devapc_vio_info *vio_info;
> +       void __iomem *vio_dbg0_reg;
> +       void __iomem *vio_dbg1_reg;
> +       u32 dbg0;
> +
> +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> +
> +       vio_dbgs = ctx->vio_dbgs;
> +       vio_info = ctx->vio_info;
> +
> +       /* Starts to extract violation information */
> +       dbg0 = readl(vio_dbg0_reg);
> +       vio_info->vio_addr = readl(vio_dbg1_reg);
> +
> +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> +                             vio_dbgs->mstid.start;

What is master_id? How could we use it to debug? For example, if we
get a master_id = 1, what should we do for this?

> +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> +                             vio_dbgs->dmnid.start;

What is domain_id? How could we use it to debug? For example, if we
get a domain_id = 2, what should we do for this?

> +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> +                           vio_dbgs->vio_w.start) == 1;
> +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> +                         vio_dbgs->vio_r.start) == 1;
> +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> +                                 vio_dbgs->addr_h.start;

What is vio_addr_high? As I know all register address are 32 bits, is
vio_addr_high the address above 32 bits?

> +
> +       devapc_vio_info_print(ctx);
> +}
> +

[snip]

> +
> +/*
> + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> + *                        violation information including which master violates
> + *                        access slave.
> + */
> +static irqreturn_t devapc_violation_irq(int irq_number,
> +                                       struct mtk_devapc_context *ctx)
> +{
> +       u32 vio_idx;
> +
> +       /*
> +        * Mask slave's irq before clearing vio status.
> +        * Must do it to avoid nested interrupt and prevent
> +        * unexpected behavior.
> +        */
> +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
> +               mask_module_irq(ctx, vio_idx, true);

I would like to rewrite this for-loop as below to prevent too many
function call in irq handler.

for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->vio_idx_num); i++)
    writel(0xffffffff, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);

reg  = readl(ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
reg |= 1 << (ctx->vio_idx_num - 32 * i + 1) - 1;
writel(reg, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);

> +
> +       devapc_dump_vio_dbg(ctx);
> +
> +       /*
> +        * Ensure that violation info are written
> +        * before further operations
> +        */
> +       smp_mb();
> +
> +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> +               clear_vio_status(ctx, vio_idx);
> +               mask_module_irq(ctx, vio_idx, false);
> +       }

Ditto for this for-loop.

> +
> +       return IRQ_HANDLED;
> +}
> +

[snip]

> +
> +static int mtk_devapc_probe(struct platform_device *pdev)
> +{
> +       struct device_node *node = pdev->dev.of_node;
> +       struct mtk_devapc_context *ctx;
> +       struct clk *devapc_infra_clk;
> +       u32 devapc_irq;
> +       int ret;
> +
> +       if (IS_ERR(node))
> +               return -ENODEV;
> +
> +       ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> +       if (!ctx)
> +               return -ENOMEM;
> +
> +       ctx = (struct mtk_devapc_context *)of_device_get_match_data(&pdev->dev);
> +       ctx->dev = &pdev->dev;
> +
> +       ctx->vio_info = devm_kzalloc(&pdev->dev,
> +                                    sizeof(struct mtk_devapc_vio_info),
> +                                    GFP_KERNEL);
> +       if (!ctx->vio_info)
> +               return -ENOMEM;
> +
> +       ctx->devapc_pd_base = of_iomap(node, 0);
> +       if (!ctx->devapc_pd_base)
> +               return -EINVAL;
> +
> +       devapc_irq = irq_of_parse_and_map(node, 0);
> +       if (!devapc_irq)
> +               return -EINVAL;
> +
> +       devapc_infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> +       if (IS_ERR(devapc_infra_clk))
> +               return -EINVAL;
> +
> +       if (clk_prepare_enable(devapc_infra_clk))
> +               return -EINVAL;
> +
> +       ret = devm_request_irq(&pdev->dev, devapc_irq,
> +                              (irq_handler_t)devapc_violation_irq,
> +                              IRQF_TRIGGER_NONE, "devapc", ctx);
> +       if (ret)

You should clk_disable_unprepare(devapc_infra_clk);

> +               return ret;
> +
> +       start_devapc(ctx);
> +
> +       return 0;
> +}
> +
> +static int mtk_devapc_remove(struct platform_device *dev)
> +{

Ditto.

Regards,
Chun-Kuang.

> +       return 0;
> +}
> +
> +static struct platform_driver mtk_devapc_driver = {
> +       .probe = mtk_devapc_probe,
> +       .remove = mtk_devapc_remove,
> +       .driver = {
> +               .name = KBUILD_MODNAME,
> +               .of_match_table = mtk_devapc_dt_match,
> +       },
> +};
> +
> +module_platform_driver(mtk_devapc_driver);
> +

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Hi, Neal:

Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>

[snip]

> +
> +static void devapc_vio_info_print(struct mtk_devapc_context *ctx)
> +{
> +       struct mtk_devapc_vio_info *vio_info = ctx->vio_info;
> +
> +       /* Print violation information */
> +       if (vio_info->write)
> +               dev_info(ctx->dev, "Write Violation\n");
> +       else if (vio_info->read)
> +               dev_info(ctx->dev, "Read Violation\n");
> +
> +       dev_info(ctx->dev, "Vio Addr:0x%x, High:0x%x, Bus ID:0x%x, Dom ID:%x\n",
> +                vio_info->vio_addr, vio_info->vio_addr_high,
> +                vio_info->master_id, vio_info->domain_id);
> +}

devapc_vio_info_print() is small function and only called by
devapc_extract_vio_dbg(), so I would like to merge this function into
devapc_extract_vio_dbg() and you could drop struct mtk_devapc_vio_info
because its member are all local variable.

> +
> +/*
> + * devapc_extract_vio_dbg - extract full violation information after doing
> + *                          shift mechanism.
> + */
> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> +{
> +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> +       struct mtk_devapc_vio_info *vio_info;
> +       void __iomem *vio_dbg0_reg;
> +       void __iomem *vio_dbg1_reg;
> +       u32 dbg0;
> +
> +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> +
> +       vio_dbgs = ctx->vio_dbgs;
> +       vio_info = ctx->vio_info;
> +
> +       /* Starts to extract violation information */
> +       dbg0 = readl(vio_dbg0_reg);
> +       vio_info->vio_addr = readl(vio_dbg1_reg);
> +
> +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> +                             vio_dbgs->mstid.start;
> +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> +                             vio_dbgs->dmnid.start;
> +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> +                           vio_dbgs->vio_w.start) == 1;
> +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> +                         vio_dbgs->vio_r.start) == 1;
> +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> +                                 vio_dbgs->addr_h.start;
> +
> +       devapc_vio_info_print(ctx);
> +}
> +

[snip]

> +
> +/*
> + * start_devapc - unmask slave's irq to start receiving devapc violation.
> + */
> +static void start_devapc(struct mtk_devapc_context *ctx)
> +{
> +       u32 vio_idx;
> +
> +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
> +               mask_module_irq(ctx, vio_idx, false);

Are these bits default true? If they are default false, you need not
to setup it to false again.

> +}
> +

[snip]

> diff --git a/drivers/soc/mediatek/mtk-devapc.h b/drivers/soc/mediatek/mtk-devapc.h
> new file mode 100644
> index 0000000..7bd7e66
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-devapc.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020 MediaTek Inc.
> + */
> +
> +#ifndef __MTK_DEVAPC_H__
> +#define __MTK_DEVAPC_H__
> +
> +#define VIO_MOD_TO_REG_IND(m)  ((m) / 32)
> +#define VIO_MOD_TO_REG_OFF(m)  ((m) % 32)
> +
> +struct mtk_devapc_pd_offset {
> +       u32 vio_mask;
> +       u32 vio_sta;
> +       u32 vio_dbg0;
> +       u32 vio_dbg1;
> +       u32 apc_con;
> +       u32 vio_shift_sta;
> +       u32 vio_shift_sel;
> +       u32 vio_shift_con;
> +};
> +
> +struct mtk_devapc_vio_dbgs_desc {
> +       u32 mask;
> +       u32 start;
> +};
> +
> +struct mtk_devapc_vio_dbgs {
> +       struct mtk_devapc_vio_dbgs_desc mstid;
> +       struct mtk_devapc_vio_dbgs_desc dmnid;
> +       struct mtk_devapc_vio_dbgs_desc vio_w;
> +       struct mtk_devapc_vio_dbgs_desc vio_r;
> +       struct mtk_devapc_vio_dbgs_desc addr_h;
> +};
> +
> +struct mtk_devapc_vio_info {
> +       bool read;
> +       bool write;
> +       u32 vio_addr;
> +       u32 vio_addr_high;
> +       u32 master_id;
> +       u32 domain_id;
> +};
> +
> +struct mtk_devapc_context {
> +       struct device *dev;
> +       u32 vio_idx_num;
> +       void __iomem *devapc_pd_base;
> +       struct mtk_devapc_vio_info *vio_info;
> +       const struct mtk_devapc_pd_offset *offset;
> +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> +};
> +
> +#endif /* __MTK_DEVAPC_H__ */

Data in this header file is only used in mtk-devapc.c and mtk-devapc.c
is a small file, so I think it's better to move data in header file
into .c file to make code simpler.

Regards,
Chun-Kuang.

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Hi, Neal:

Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> ---

[snip]

> +
> +/*
> + * devapc_extract_vio_dbg - extract full violation information after doing
> + *                          shift mechanism.
> + */
> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> +{
> +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> +       struct mtk_devapc_vio_info *vio_info;
> +       void __iomem *vio_dbg0_reg;
> +       void __iomem *vio_dbg1_reg;
> +       u32 dbg0;
> +
> +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> +
> +       vio_dbgs = ctx->vio_dbgs;
> +       vio_info = ctx->vio_info;
> +
> +       /* Starts to extract violation information */
> +       dbg0 = readl(vio_dbg0_reg);
> +       vio_info->vio_addr = readl(vio_dbg1_reg);
> +
> +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> +                             vio_dbgs->mstid.start;
> +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> +                             vio_dbgs->dmnid.start;
> +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> +                           vio_dbgs->vio_w.start) == 1;
> +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> +                         vio_dbgs->vio_r.start) == 1;
> +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> +                                 vio_dbgs->addr_h.start;


I would like to define the type of ctx->vio_info to be

struct mtk_devapc_vio_dbgs {
    u32 mstid:16;
    u32 dmnid:6;
    u32 vio_w:1;
    u32 vio_r:1;
    u32 addr_h:4;
    u32 resv:4;
};

so the code would like the simple way

ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);

Regards,
Chun-Kuang.

> +
> +       devapc_vio_info_print(ctx);
> +}
> +

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

Hi Chun-Kuang,


On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > ---
> 
> [snip]
> 
> > +
> > +static int get_shift_group(struct mtk_devapc_context *ctx)
> > +{
> > +       u32 vio_shift_sta;
> > +       void __iomem *reg;
> > +
> > +       reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > +       vio_shift_sta = readl(reg);
> > +
> > +       if (vio_shift_sta)
> > +               return __ffs(vio_shift_sta);
> > +
> > +       return -EIO;
> > +}
> 
> get_shift_group() is a small function, I would like to merge this
> function into sync_vio_dbg() to make code more simple.

This function have a specific functionality. And it would make this
driver more readability. I would like to keep it as a function. Is that
okay for you?

> 
> > +
> 
> [snip]
> 
> > +
> > +#define PHY_DEVAPC_TIMEOUT     0x10000
> > +
> > +/*
> > + * sync_vio_dbg - do "shift" mechansim" to get full violation information.
> > + *                shift mechanism is depends on devapc hardware design.
> > + *                Mediatek devapc set multiple slaves as a group. When violation
> > + *                is triggered, violation info is kept inside devapc hardware.
> > + *                Driver should do shift mechansim to "shift" full violation
> > + *                info to VIO_DBGs registers.
> > + *
> > + */
> > +static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
> > +{
> > +       void __iomem *pd_vio_shift_sta_reg;
> > +       void __iomem *pd_vio_shift_sel_reg;
> > +       void __iomem *pd_vio_shift_con_reg;
> > +       int ret;
> > +       u32 val;
> > +
> > +       pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > +       pd_vio_shift_sel_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sel;
> > +       pd_vio_shift_con_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_con;
> > +
> > +       /* Enable shift mechansim */
> > +       writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
> > +       writel(0x1, pd_vio_shift_con_reg);
> > +
> > +       ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> > +                                PHY_DEVAPC_TIMEOUT);
> > +       if (ret)
> > +               dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
> > +
> > +       /* Disable shift mechanism */
> > +       writel(0x0, pd_vio_shift_con_reg);
> > +       writel(0x0, pd_vio_shift_sel_reg);
> > +       writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
> > +
> > +       return ret;
> > +}
> > +
> 
> [snip]
> 
> > +
> > +/*
> > + * devapc_extract_vio_dbg - extract full violation information after doing
> > + *                          shift mechanism.
> > + */
> > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > +       struct mtk_devapc_vio_info *vio_info;
> > +       void __iomem *vio_dbg0_reg;
> > +       void __iomem *vio_dbg1_reg;
> > +       u32 dbg0;
> > +
> > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > +
> > +       vio_dbgs = ctx->vio_dbgs;
> > +       vio_info = ctx->vio_info;
> > +
> > +       /* Starts to extract violation information */
> > +       dbg0 = readl(vio_dbg0_reg);
> > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > +
> > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > +                             vio_dbgs->mstid.start;
> 
> What is master_id? How could we use it to debug? For example, if we
> get a master_id = 1, what should we do for this?
> 
> > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > +                             vio_dbgs->dmnid.start;
> 
> What is domain_id? How could we use it to debug? For example, if we
> get a domain_id = 2, what should we do for this?
> 

master_id and domain_id belongs our bus side-band signal info. It can
help us to find the violation master.

> > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > +                           vio_dbgs->vio_w.start) == 1;
> > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > +                         vio_dbgs->vio_r.start) == 1;
> > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > +                                 vio_dbgs->addr_h.start;
> 
> What is vio_addr_high? As I know all register address are 32 bits, is
> vio_addr_high the address above 32 bits?

Yes, you are right. In MT6779, all register base are 32 bits. We can
ignore this info for current driver. I'll update on next patch.
Thanks !

> 
> > +
> > +       devapc_vio_info_print(ctx);
> > +}
> > +
> 
> [snip]
> 
> > +
> > +/*
> > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > + *                        violation information including which master violates
> > + *                        access slave.
> > + */
> > +static irqreturn_t devapc_violation_irq(int irq_number,
> > +                                       struct mtk_devapc_context *ctx)
> > +{
> > +       u32 vio_idx;
> > +
> > +       /*
> > +        * Mask slave's irq before clearing vio status.
> > +        * Must do it to avoid nested interrupt and prevent
> > +        * unexpected behavior.
> > +        */
> > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
> > +               mask_module_irq(ctx, vio_idx, true);
> 
> I would like to rewrite this for-loop as below to prevent too many
> function call in irq handler.
> 
> for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->vio_idx_num); i++)
>     writel(0xffffffff, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
> 

This idea is okay for me. Is there any macro to replace 0xffffffff?

> reg  = readl(ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
> reg |= 1 << (ctx->vio_idx_num - 32 * i + 1) - 1;
> writel(reg, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);

Are you trying to clear the bits which over vio_idx_num?
If yes, I think the second line should be:
reg &= 1 << (ctx->vio_idx_num - 32 * i) - 1;

For example, if vio_idx_num is 40:
after for loop:
vio_mask0 = 0xffffffff;
vio_mask1 = 0xffffffff;

reg = readl(vio_mask1);
reg &= (1 << 8) - 1; (which is 0x000000ff)
reg will be 0xff
writel(reg, vio_mask1);

Does it make sense?

Actually, it is okay to overwrite the unused register bits.
So it's no matter to do this step.

> 
> > +
> > +       devapc_dump_vio_dbg(ctx);
> > +
> > +       /*
> > +        * Ensure that violation info are written
> > +        * before further operations
> > +        */
> > +       smp_mb();
> > +
> > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > +               clear_vio_status(ctx, vio_idx);
> > +               mask_module_irq(ctx, vio_idx, false);
> > +       }
> 
> Ditto for this for-loop.

Ditto.

> 
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> 
> [snip]
> 
> > +
> > +static int mtk_devapc_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *node = pdev->dev.of_node;
> > +       struct mtk_devapc_context *ctx;
> > +       struct clk *devapc_infra_clk;
> > +       u32 devapc_irq;
> > +       int ret;
> > +
> > +       if (IS_ERR(node))
> > +               return -ENODEV;
> > +
> > +       ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> > +       if (!ctx)
> > +               return -ENOMEM;
> > +
> > +       ctx = (struct mtk_devapc_context *)of_device_get_match_data(&pdev->dev);
> > +       ctx->dev = &pdev->dev;
> > +
> > +       ctx->vio_info = devm_kzalloc(&pdev->dev,
> > +                                    sizeof(struct mtk_devapc_vio_info),
> > +                                    GFP_KERNEL);
> > +       if (!ctx->vio_info)
> > +               return -ENOMEM;
> > +
> > +       ctx->devapc_pd_base = of_iomap(node, 0);
> > +       if (!ctx->devapc_pd_base)
> > +               return -EINVAL;
> > +
> > +       devapc_irq = irq_of_parse_and_map(node, 0);
> > +       if (!devapc_irq)
> > +               return -EINVAL;
> > +
> > +       devapc_infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> > +       if (IS_ERR(devapc_infra_clk))
> > +               return -EINVAL;
> > +
> > +       if (clk_prepare_enable(devapc_infra_clk))
> > +               return -EINVAL;
> > +
> > +       ret = devm_request_irq(&pdev->dev, devapc_irq,
> > +                              (irq_handler_t)devapc_violation_irq,
> > +                              IRQF_TRIGGER_NONE, "devapc", ctx);
> > +       if (ret)
> 
> You should clk_disable_unprepare(devapc_infra_clk);

Yes, I miss this part. Thanks for your remind.
I'll update it on next patch.

> 
> > +               return ret;
> > +
> > +       start_devapc(ctx);
> > +
> > +       return 0;
> > +}
> > +
> > +static int mtk_devapc_remove(struct platform_device *dev)
> > +{
> 
> Ditto.

Ditto.

> 
> Regards,
> Chun-Kuang.
> 
> > +       return 0;
> > +}
> > +
> > +static struct platform_driver mtk_devapc_driver = {
> > +       .probe = mtk_devapc_probe,
> > +       .remove = mtk_devapc_remove,
> > +       .driver = {
> > +               .name = KBUILD_MODNAME,
> > +               .of_match_table = mtk_devapc_dt_match,
> > +       },
> > +};
> > +
> > +module_platform_driver(mtk_devapc_driver);
> > +

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

Hi Chun-Kuang,


On Thu, 2020-07-30 at 06:47 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> 
> [snip]
> 
> > +
> > +static void devapc_vio_info_print(struct mtk_devapc_context *ctx)
> > +{
> > +       struct mtk_devapc_vio_info *vio_info = ctx->vio_info;
> > +
> > +       /* Print violation information */
> > +       if (vio_info->write)
> > +               dev_info(ctx->dev, "Write Violation\n");
> > +       else if (vio_info->read)
> > +               dev_info(ctx->dev, "Read Violation\n");
> > +
> > +       dev_info(ctx->dev, "Vio Addr:0x%x, High:0x%x, Bus ID:0x%x, Dom ID:%x\n",
> > +                vio_info->vio_addr, vio_info->vio_addr_high,
> > +                vio_info->master_id, vio_info->domain_id);
> > +}
> 
> devapc_vio_info_print() is small function and only called by
> devapc_extract_vio_dbg(), so I would like to merge this function into
> devapc_extract_vio_dbg() and you could drop struct mtk_devapc_vio_info
> because its member are all local variable.

This idea is okay for me. I'll update on next patch.
Thanks !

> 
> > +
> > +/*
> > + * devapc_extract_vio_dbg - extract full violation information after doing
> > + *                          shift mechanism.
> > + */
> > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > +       struct mtk_devapc_vio_info *vio_info;
> > +       void __iomem *vio_dbg0_reg;
> > +       void __iomem *vio_dbg1_reg;
> > +       u32 dbg0;
> > +
> > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > +
> > +       vio_dbgs = ctx->vio_dbgs;
> > +       vio_info = ctx->vio_info;
> > +
> > +       /* Starts to extract violation information */
> > +       dbg0 = readl(vio_dbg0_reg);
> > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > +
> > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > +                             vio_dbgs->mstid.start;
> > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > +                             vio_dbgs->dmnid.start;
> > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > +                           vio_dbgs->vio_w.start) == 1;
> > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > +                         vio_dbgs->vio_r.start) == 1;
> > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > +                                 vio_dbgs->addr_h.start;
> > +
> > +       devapc_vio_info_print(ctx);
> > +}
> > +
> 
> [snip]
> 
> > +
> > +/*
> > + * start_devapc - unmask slave's irq to start receiving devapc violation.
> > + */
> > +static void start_devapc(struct mtk_devapc_context *ctx)
> > +{
> > +       u32 vio_idx;
> > +
> > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
> > +               mask_module_irq(ctx, vio_idx, false);
> 
> Are these bits default true? If they are default false, you need not
> to setup it to false again.

It's default value is true, which is mask.
We try to unmask it to start service.

> 
> > +}
> > +
> 
> [snip]
> 
> > diff --git a/drivers/soc/mediatek/mtk-devapc.h b/drivers/soc/mediatek/mtk-devapc.h
> > new file mode 100644
> > index 0000000..7bd7e66
> > --- /dev/null
> > +++ b/drivers/soc/mediatek/mtk-devapc.h
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2020 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_DEVAPC_H__
> > +#define __MTK_DEVAPC_H__
> > +
> > +#define VIO_MOD_TO_REG_IND(m)  ((m) / 32)
> > +#define VIO_MOD_TO_REG_OFF(m)  ((m) % 32)
> > +
> > +struct mtk_devapc_pd_offset {
> > +       u32 vio_mask;
> > +       u32 vio_sta;
> > +       u32 vio_dbg0;
> > +       u32 vio_dbg1;
> > +       u32 apc_con;
> > +       u32 vio_shift_sta;
> > +       u32 vio_shift_sel;
> > +       u32 vio_shift_con;
> > +};
> > +
> > +struct mtk_devapc_vio_dbgs_desc {
> > +       u32 mask;
> > +       u32 start;
> > +};
> > +
> > +struct mtk_devapc_vio_dbgs {
> > +       struct mtk_devapc_vio_dbgs_desc mstid;
> > +       struct mtk_devapc_vio_dbgs_desc dmnid;
> > +       struct mtk_devapc_vio_dbgs_desc vio_w;
> > +       struct mtk_devapc_vio_dbgs_desc vio_r;
> > +       struct mtk_devapc_vio_dbgs_desc addr_h;
> > +};
> > +
> > +struct mtk_devapc_vio_info {
> > +       bool read;
> > +       bool write;
> > +       u32 vio_addr;
> > +       u32 vio_addr_high;
> > +       u32 master_id;
> > +       u32 domain_id;
> > +};
> > +
> > +struct mtk_devapc_context {
> > +       struct device *dev;
> > +       u32 vio_idx_num;
> > +       void __iomem *devapc_pd_base;
> > +       struct mtk_devapc_vio_info *vio_info;
> > +       const struct mtk_devapc_pd_offset *offset;
> > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > +};
> > +
> > +#endif /* __MTK_DEVAPC_H__ */
> 
> Data in this header file is only used in mtk-devapc.c and mtk-devapc.c
> is a small file, so I think it's better to move data in header file
> into .c file to make code simpler.

This idea is okay for me. I'll update on next patch.
Thanks !

> 
> Regards,
> Chun-Kuang.

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

Hi Chun-Kuang,

On Fri, 2020-07-31 at 00:14 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > ---
> 
> [snip]
> 
> > +
> > +/*
> > + * devapc_extract_vio_dbg - extract full violation information after doing
> > + *                          shift mechanism.
> > + */
> > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > +       struct mtk_devapc_vio_info *vio_info;
> > +       void __iomem *vio_dbg0_reg;
> > +       void __iomem *vio_dbg1_reg;
> > +       u32 dbg0;
> > +
> > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > +
> > +       vio_dbgs = ctx->vio_dbgs;
> > +       vio_info = ctx->vio_info;
> > +
> > +       /* Starts to extract violation information */
> > +       dbg0 = readl(vio_dbg0_reg);
> > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > +
> > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > +                             vio_dbgs->mstid.start;
> > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > +                             vio_dbgs->dmnid.start;
> > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > +                           vio_dbgs->vio_w.start) == 1;
> > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > +                         vio_dbgs->vio_r.start) == 1;
> > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > +                                 vio_dbgs->addr_h.start;
> 
> 
> I would like to define the type of ctx->vio_info to be
> 
> struct mtk_devapc_vio_dbgs {
>     u32 mstid:16;
>     u32 dmnid:6;
>     u32 vio_w:1;
>     u32 vio_r:1;
>     u32 addr_h:4;
>     u32 resv:4;
> };
> 
> so the code would like the simple way
> 
> ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
> 

This idea looks great! Is there any possible to pass the bit layout by
DT data, and still make this operation simple?
Why am I asking this question is because this bit layout is platform
dependent.

> Regards,
> Chun-Kuang.
> 
> > +
> > +       devapc_vio_info_print(ctx);
> > +}
> > +

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Hi, Neal:

Neal Liu <neal.liu@mediatek.com> 於 2020年7月31日 週五 上午10:44寫道：
>
> Hi Chun-Kuang,
>
>
> On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > >
> > > MediaTek bus fabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violation is logged and sent to the processor for
> > > further analysis or countermeasures.
> > >
> > > Any occurrence of security violation would raise an interrupt, and
> > > it will be handled by mtk-devapc driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > ---
> >
> > [snip]
> >
> > > +
> > > +static int get_shift_group(struct mtk_devapc_context *ctx)
> > > +{
> > > +       u32 vio_shift_sta;
> > > +       void __iomem *reg;
> > > +
> > > +       reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > > +       vio_shift_sta = readl(reg);
> > > +
> > > +       if (vio_shift_sta)
> > > +               return __ffs(vio_shift_sta);
> > > +
> > > +       return -EIO;
> > > +}
> >
> > get_shift_group() is a small function, I would like to merge this
> > function into sync_vio_dbg() to make code more simple.
>
> This function have a specific functionality. And it would make this
> driver more readability. I would like to keep it as a function. Is that
> okay for you?

After merge, the function would be:

static bool sync_min_shift_group_vio_dbg(struct mtk_devapc_context *ctx)
{
 int min_shift_group;
 int ret;
 u32 val;

 /* find the minimum shift group which has violation */
 val = readl(ctx->devapc_pd_base + ctx->offset->vio_shift_sta);
 if (!val)
    return false;

 min_shift_group = __ffs(val);

 /* Assign the group to sync */
 writel(0x1 << min_shift_group, ctx->devapc_pd_base +
ctx->offset->vio_shift_sel);

 /* Start syncing */
 writel(0x1, ctx->devapc_pd_base + ctx->offset->vio_shift_con);

 ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
     PHY_DEVAPC_TIMEOUT);
 if (ret) {
  dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
  return false;
 }

 /* Stop syncing */
 writel(0x0, ctx->devapc_pd_base + ctx->offset->vio_shift_con);

 /* ? */
 writel(0x1 << min_shift_group, ctx->devapc_pd_base +
ctx->offset->vio_shift_sta);

 return true;
}

The whole function is to sync min_shift_group violation info, I don't
know why separate any part to an independent function? Any function
call would cause penalty on CPU performance, so I does not like to
break this function. After good comment, I think every body could
understand the function of each register.
After the merge, the code would be so simple as:

while(sync_min_shift_group_vio_dbg(ctx))
    devapc_extract_vio_dbg(ctx);

>
> >
> > > +
> >
> > [snip]
> >
> > > +
> > > +#define PHY_DEVAPC_TIMEOUT     0x10000
> > > +
> > > +/*
> > > + * sync_vio_dbg - do "shift" mechansim" to get full violation information.
> > > + *                shift mechanism is depends on devapc hardware design.
> > > + *                Mediatek devapc set multiple slaves as a group. When violation
> > > + *                is triggered, violation info is kept inside devapc hardware.
> > > + *                Driver should do shift mechansim to "shift" full violation
> > > + *                info to VIO_DBGs registers.
> > > + *
> > > + */
> > > +static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
> > > +{
> > > +       void __iomem *pd_vio_shift_sta_reg;
> > > +       void __iomem *pd_vio_shift_sel_reg;
> > > +       void __iomem *pd_vio_shift_con_reg;
> > > +       int ret;
> > > +       u32 val;
> > > +
> > > +       pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > > +       pd_vio_shift_sel_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sel;
> > > +       pd_vio_shift_con_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_con;
> > > +
> > > +       /* Enable shift mechansim */
> > > +       writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
> > > +       writel(0x1, pd_vio_shift_con_reg);
> > > +
> > > +       ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> > > +                                PHY_DEVAPC_TIMEOUT);
> > > +       if (ret)
> > > +               dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
> > > +
> > > +       /* Disable shift mechanism */
> > > +       writel(0x0, pd_vio_shift_con_reg);
> > > +       writel(0x0, pd_vio_shift_sel_reg);
> > > +       writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
> > > +
> > > +       return ret;
> > > +}
> > > +
> >
> > [snip]
> >
> > > +
> > > +/*
> > > + * devapc_extract_vio_dbg - extract full violation information after doing
> > > + *                          shift mechanism.
> > > + */
> > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > +{
> > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > +       struct mtk_devapc_vio_info *vio_info;
> > > +       void __iomem *vio_dbg0_reg;
> > > +       void __iomem *vio_dbg1_reg;
> > > +       u32 dbg0;
> > > +
> > > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > +
> > > +       vio_dbgs = ctx->vio_dbgs;
> > > +       vio_info = ctx->vio_info;
> > > +
> > > +       /* Starts to extract violation information */
> > > +       dbg0 = readl(vio_dbg0_reg);
> > > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > > +
> > > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > > +                             vio_dbgs->mstid.start;
> >
> > What is master_id? How could we use it to debug? For example, if we
> > get a master_id = 1, what should we do for this?
> >
> > > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > +                             vio_dbgs->dmnid.start;
> >
> > What is domain_id? How could we use it to debug? For example, if we
> > get a domain_id = 2, what should we do for this?
> >
>
> master_id and domain_id belongs our bus side-band signal info. It can
> help us to find the violation master.

Does 'violation master' means the hardware could access the protected
register? (ex. CPU, GCE, ...) If so, I think it's better to add
comment to explain how to map (master_id, domain_id) to a hardware
(maybe the device in device tree) because every body does not know
what the number means. Don't try to translate the number to a string
because this would cost much time to do this. Just print a number and
we could find out the master by the comment.

>
> > > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > +                           vio_dbgs->vio_w.start) == 1;
> > > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > +                         vio_dbgs->vio_r.start) == 1;
> > > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > +                                 vio_dbgs->addr_h.start;
> >
> > What is vio_addr_high? As I know all register address are 32 bits, is
> > vio_addr_high the address above 32 bits?
>
> Yes, you are right. In MT6779, all register base are 32 bits. We can
> ignore this info for current driver. I'll update on next patch.
> Thanks !

Such a strange hardware, all register is 32 bits but it has a
vio_addr_high in its register. OK, just drop this.

>
> >
> > > +
> > > +       devapc_vio_info_print(ctx);
> > > +}
> > > +
> >
> > [snip]
> >
> > > +
> > > +/*
> > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > + *                        violation information including which master violates
> > > + *                        access slave.
> > > + */
> > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > +                                       struct mtk_devapc_context *ctx)
> > > +{
> > > +       u32 vio_idx;
> > > +
> > > +       /*
> > > +        * Mask slave's irq before clearing vio status.
> > > +        * Must do it to avoid nested interrupt and prevent
> > > +        * unexpected behavior.
> > > +        */
> > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
> > > +               mask_module_irq(ctx, vio_idx, true);
> >
> > I would like to rewrite this for-loop as below to prevent too many
> > function call in irq handler.
> >
> > for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->vio_idx_num); i++)
> >     writel(0xffffffff, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
> >
>
> This idea is okay for me. Is there any macro to replace 0xffffffff?

GENMASK(31, 0);

>
> > reg  = readl(ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
> > reg |= 1 << (ctx->vio_idx_num - 32 * i + 1) - 1;
> > writel(reg, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
>
> Are you trying to clear the bits which over vio_idx_num?
> If yes, I think the second line should be:
> reg &= 1 << (ctx->vio_idx_num - 32 * i) - 1;
>
> For example, if vio_idx_num is 40:
> after for loop:
> vio_mask0 = 0xffffffff;
> vio_mask1 = 0xffffffff;

when vio_idx_num is 40, VIO_MOD_TO_REG_IND(ctx->vio_idx_num) is 1, so
after for-loop:
vio_mask0 = 0xffffffff;

And the code after for-loop just to do this:
vio_mask1 = 0xff;

>
> reg = readl(vio_mask1);
> reg &= (1 << 8) - 1; (which is 0x000000ff)
> reg will be 0xff
> writel(reg, vio_mask1);
>
> Does it make sense?
>
> Actually, it is okay to overwrite the unused register bits.
> So it's no matter to do this step.

OK, the code would be

for (i = 0; i <= VIO_MOD_TO_REG_IND(ctx->vio_idx_num - 1); i++)
    writel(GENMASK(31, 0), ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);

Regards,
Chun-Kuang.

>
> >
> > > +
> > > +       devapc_dump_vio_dbg(ctx);
> > > +
> > > +       /*
> > > +        * Ensure that violation info are written
> > > +        * before further operations
> > > +        */
> > > +       smp_mb();
> > > +
> > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > +               clear_vio_status(ctx, vio_idx);
> > > +               mask_module_irq(ctx, vio_idx, false);
> > > +       }
> >
> > Ditto for this for-loop.
>
> Ditto.
>
> >
> > > +
> > > +       return IRQ_HANDLED;
> > > +}
> > > +
> >
> > [snip]
> >
> > > +
> > > +static int mtk_devapc_probe(struct platform_device *pdev)
> > > +{
> > > +       struct device_node *node = pdev->dev.of_node;
> > > +       struct mtk_devapc_context *ctx;
> > > +       struct clk *devapc_infra_clk;
> > > +       u32 devapc_irq;
> > > +       int ret;
> > > +
> > > +       if (IS_ERR(node))
> > > +               return -ENODEV;
> > > +
> > > +       ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> > > +       if (!ctx)
> > > +               return -ENOMEM;
> > > +
> > > +       ctx = (struct mtk_devapc_context *)of_device_get_match_data(&pdev->dev);
> > > +       ctx->dev = &pdev->dev;
> > > +
> > > +       ctx->vio_info = devm_kzalloc(&pdev->dev,
> > > +                                    sizeof(struct mtk_devapc_vio_info),
> > > +                                    GFP_KERNEL);
> > > +       if (!ctx->vio_info)
> > > +               return -ENOMEM;
> > > +
> > > +       ctx->devapc_pd_base = of_iomap(node, 0);
> > > +       if (!ctx->devapc_pd_base)
> > > +               return -EINVAL;
> > > +
> > > +       devapc_irq = irq_of_parse_and_map(node, 0);
> > > +       if (!devapc_irq)
> > > +               return -EINVAL;
> > > +
> > > +       devapc_infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> > > +       if (IS_ERR(devapc_infra_clk))
> > > +               return -EINVAL;
> > > +
> > > +       if (clk_prepare_enable(devapc_infra_clk))
> > > +               return -EINVAL;
> > > +
> > > +       ret = devm_request_irq(&pdev->dev, devapc_irq,
> > > +                              (irq_handler_t)devapc_violation_irq,
> > > +                              IRQF_TRIGGER_NONE, "devapc", ctx);
> > > +       if (ret)
> >
> > You should clk_disable_unprepare(devapc_infra_clk);
>
> Yes, I miss this part. Thanks for your remind.
> I'll update it on next patch.
>
> >
> > > +               return ret;
> > > +
> > > +       start_devapc(ctx);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int mtk_devapc_remove(struct platform_device *dev)
> > > +{
> >
> > Ditto.
>
> Ditto.
>
> >
> > Regards,
> > Chun-Kuang.
> >
> > > +       return 0;
> > > +}
> > > +
> > > +static struct platform_driver mtk_devapc_driver = {
> > > +       .probe = mtk_devapc_probe,
> > > +       .remove = mtk_devapc_remove,
> > > +       .driver = {
> > > +               .name = KBUILD_MODNAME,
> > > +               .of_match_table = mtk_devapc_dt_match,
> > > +       },
> > > +};
> > > +
> > > +module_platform_driver(mtk_devapc_driver);
> > > +
>

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Hi, Neal:

Neal Liu <neal.liu@mediatek.com> 於 2020年7月31日 週五 上午10:52寫道：
>
> Hi Chun-Kuang,
>
> On Fri, 2020-07-31 at 00:14 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > >
> > > MediaTek bus fabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violation is logged and sent to the processor for
> > > further analysis or countermeasures.
> > >
> > > Any occurrence of security violation would raise an interrupt, and
> > > it will be handled by mtk-devapc driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > ---
> >
> > [snip]
> >
> > > +
> > > +/*
> > > + * devapc_extract_vio_dbg - extract full violation information after doing
> > > + *                          shift mechanism.
> > > + */
> > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > +{
> > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > +       struct mtk_devapc_vio_info *vio_info;
> > > +       void __iomem *vio_dbg0_reg;
> > > +       void __iomem *vio_dbg1_reg;
> > > +       u32 dbg0;
> > > +
> > > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > +
> > > +       vio_dbgs = ctx->vio_dbgs;
> > > +       vio_info = ctx->vio_info;
> > > +
> > > +       /* Starts to extract violation information */
> > > +       dbg0 = readl(vio_dbg0_reg);
> > > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > > +
> > > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > > +                             vio_dbgs->mstid.start;
> > > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > +                             vio_dbgs->dmnid.start;
> > > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > +                           vio_dbgs->vio_w.start) == 1;
> > > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > +                         vio_dbgs->vio_r.start) == 1;
> > > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > +                                 vio_dbgs->addr_h.start;
> >
> >
> > I would like to define the type of ctx->vio_info to be
> >
> > struct mtk_devapc_vio_dbgs {
> >     u32 mstid:16;
> >     u32 dmnid:6;
> >     u32 vio_w:1;
> >     u32 vio_r:1;
> >     u32 addr_h:4;
> >     u32 resv:4;
> > };
> >
> > so the code would like the simple way
> >
> > ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
> >
>
> This idea looks great! Is there any possible to pass the bit layout by
> DT data, and still make this operation simple?
> Why am I asking this question is because this bit layout is platform
> dependent.

I doubt these info would be in a single 32-bits register for all
future SoC. If they are not in single 32-bits register, you may create
a vio_dbgs_type in DT data, and the code may be

if (ctx->vio_dbgs_type == VIO_DBGS_TYPE_MTxxxx) {
    ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
} else if (ctx->vio_dbgs_type == VIO_DBGS_TYPE_MTyyyy) {
    ctx->vio_info->mstid = readl(vio_mstid_reg);
    ctx->vio_info->dmnid = readl(vio_dmnid_reg);
    ctx->vio_info->vio_w = readl(vio_vio_w_reg);
    ctx->vio_info->vio_r = readl(vio_vio_r_reg);
}

I think we need not to consider how the future would be. Once the
second SoC driver is upstreaming, we could find out the best solution
for it.

Regards,
Chun-Kuang.

>
> > Regards,
> > Chun-Kuang.
> >
> > > +
> > > +       devapc_vio_info_print(ctx);
> > > +}
> > > +
>

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Hi, Neal:

This patch is for "mediatek,mt6779-devapc", so I think commit title
should show the SoC ID.

Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> ---

[snip]

> +
> +struct mtk_devapc_context {
> +       struct device *dev;
> +       u32 vio_idx_num;
> +       void __iomem *devapc_pd_base;
> +       struct mtk_devapc_vio_info *vio_info;
> +       const struct mtk_devapc_pd_offset *offset;
> +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> +};

I think this structure should separate the constant part. The constant part is:

struct mtk_devapc_data {
    const u32 vio_idx_num;
    const struct mtk_devapc_pd_offset *offset; /* I would like to
remove struct mtk_devapc_pd_offset and directly put its member into
this structure */
    const struct mtk_devapc_vio_dbgs *vio_dbgs; /* This may disappear */
};

And the context is:

struct mtk_devapc_context {
    struct device *dev;
    void __iomem *devapc_pd_base;
    const struct mtk_devapc_data *data;
};

So when you define this, you would not waste memory to store non-constant data.

static const struct mtk_devapc_data devapc_mt6779 = {
 .vio_idx_num = 510,
 .offset = &mt6779_pd_offset,
 .vio_dbgs = &mt6779_vio_dbgs,
};

Regards,
Chun-Kuang.

> +
> +#endif /* __MTK_DEVAPC_H__ */
> --
> 1.7.9.5
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Hi, Neal:

Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> ---

[snip]

> +
> +struct mtk_devapc_context {
> +       struct device *dev;
> +       u32 vio_idx_num;
> +       void __iomem *devapc_pd_base;

This is devapc context, so prefix 'devapc' is redundant.
And, what does 'pd' mean?

Regards,
Chun-Kuang.

> +       struct mtk_devapc_vio_info *vio_info;
> +       const struct mtk_devapc_pd_offset *offset;
> +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> +};
> +

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

Hi Chun-Kuang,

On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年7月31日 週五 上午10:44寫道：
> >
> > Hi Chun-Kuang,
> >
> >
> > On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > > >
> > > > MediaTek bus fabric provides TrustZone security support and data
> > > > protection to prevent slaves from being accessed by unexpected
> > > > masters.
> > > > The security violation is logged and sent to the processor for
> > > > further analysis or countermeasures.
> > > >
> > > > Any occurrence of security violation would raise an interrupt, and
> > > > it will be handled by mtk-devapc driver. The violation
> > > > information is printed in order to find the murderer.
> > > >
> > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > > ---
> > >
> > > [snip]
> > >
> > > > +
> > > > +static int get_shift_group(struct mtk_devapc_context *ctx)
> > > > +{
> > > > +       u32 vio_shift_sta;
> > > > +       void __iomem *reg;
> > > > +
> > > > +       reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > > > +       vio_shift_sta = readl(reg);
> > > > +
> > > > +       if (vio_shift_sta)
> > > > +               return __ffs(vio_shift_sta);
> > > > +
> > > > +       return -EIO;
> > > > +}
> > >
> > > get_shift_group() is a small function, I would like to merge this
> > > function into sync_vio_dbg() to make code more simple.
> >
> > This function have a specific functionality. And it would make this
> > driver more readability. I would like to keep it as a function. Is that
> > okay for you?
> 
> After merge, the function would be:
> 
> static bool sync_min_shift_group_vio_dbg(struct mtk_devapc_context *ctx)
> {
>  int min_shift_group;
>  int ret;
>  u32 val;
> 
>  /* find the minimum shift group which has violation */
>  val = readl(ctx->devapc_pd_base + ctx->offset->vio_shift_sta);
>  if (!val)
>     return false;
> 
>  min_shift_group = __ffs(val);
> 
>  /* Assign the group to sync */
>  writel(0x1 << min_shift_group, ctx->devapc_pd_base +
> ctx->offset->vio_shift_sel);
> 
>  /* Start syncing */
>  writel(0x1, ctx->devapc_pd_base + ctx->offset->vio_shift_con);
> 
>  ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
>      PHY_DEVAPC_TIMEOUT);
>  if (ret) {
>   dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
>   return false;
>  }
> 
>  /* Stop syncing */
>  writel(0x0, ctx->devapc_pd_base + ctx->offset->vio_shift_con);
> 
>  /* ? */
>  writel(0x1 << min_shift_group, ctx->devapc_pd_base +
> ctx->offset->vio_shift_sta);
> 
>  return true;
> }
> 
> The whole function is to sync min_shift_group violation info, I don't
> know why separate any part to an independent function? Any function
> call would cause penalty on CPU performance, so I does not like to
> break this function. After good comment, I think every body could
> understand the function of each register.
> After the merge, the code would be so simple as:
> 
> while(sync_min_shift_group_vio_dbg(ctx))
>     devapc_extract_vio_dbg(ctx);
> 

Okay, this looks good to me. I'll apply this on next patch.
Thanks !

> >
> > >
> > > > +
> > >
> > > [snip]
> > >
> > > > +
> > > > +#define PHY_DEVAPC_TIMEOUT     0x10000
> > > > +
> > > > +/*
> > > > + * sync_vio_dbg - do "shift" mechansim" to get full violation information.
> > > > + *                shift mechanism is depends on devapc hardware design.
> > > > + *                Mediatek devapc set multiple slaves as a group. When violation
> > > > + *                is triggered, violation info is kept inside devapc hardware.
> > > > + *                Driver should do shift mechansim to "shift" full violation
> > > > + *                info to VIO_DBGs registers.
> > > > + *
> > > > + */
> > > > +static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
> > > > +{
> > > > +       void __iomem *pd_vio_shift_sta_reg;
> > > > +       void __iomem *pd_vio_shift_sel_reg;
> > > > +       void __iomem *pd_vio_shift_con_reg;
> > > > +       int ret;
> > > > +       u32 val;
> > > > +
> > > > +       pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > > > +       pd_vio_shift_sel_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sel;
> > > > +       pd_vio_shift_con_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_con;
> > > > +
> > > > +       /* Enable shift mechansim */
> > > > +       writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
> > > > +       writel(0x1, pd_vio_shift_con_reg);
> > > > +
> > > > +       ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> > > > +                                PHY_DEVAPC_TIMEOUT);
> > > > +       if (ret)
> > > > +               dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
> > > > +
> > > > +       /* Disable shift mechanism */
> > > > +       writel(0x0, pd_vio_shift_con_reg);
> > > > +       writel(0x0, pd_vio_shift_sel_reg);
> > > > +       writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > >
> > > [snip]
> > >
> > > > +
> > > > +/*
> > > > + * devapc_extract_vio_dbg - extract full violation information after doing
> > > > + *                          shift mechanism.
> > > > + */
> > > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > > +{
> > > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > +       struct mtk_devapc_vio_info *vio_info;
> > > > +       void __iomem *vio_dbg0_reg;
> > > > +       void __iomem *vio_dbg1_reg;
> > > > +       u32 dbg0;
> > > > +
> > > > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > > +
> > > > +       vio_dbgs = ctx->vio_dbgs;
> > > > +       vio_info = ctx->vio_info;
> > > > +
> > > > +       /* Starts to extract violation information */
> > > > +       dbg0 = readl(vio_dbg0_reg);
> > > > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > > > +
> > > > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > > > +                             vio_dbgs->mstid.start;
> > >
> > > What is master_id? How could we use it to debug? For example, if we
> > > get a master_id = 1, what should we do for this?
> > >
> > > > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > > +                             vio_dbgs->dmnid.start;
> > >
> > > What is domain_id? How could we use it to debug? For example, if we
> > > get a domain_id = 2, what should we do for this?
> > >
> >
> > master_id and domain_id belongs our bus side-band signal info. It can
> > help us to find the violation master.
> 
> Does 'violation master' means the hardware could access the protected
> register? (ex. CPU, GCE, ...) If so, I think it's better to add
> comment to explain how to map (master_id, domain_id) to a hardware
> (maybe the device in device tree) because every body does not know
> what the number means. Don't try to translate the number to a string
> because this would cost much time to do this. Just print a number and
> we could find out the master by the comment.

'violation master' means the master which violates the permission
control. For example, if we set permission 'Secure R/W only' as CPU to
spi register. When violation is triggered, it means CPU access spi
register through normal world instead of secure world, which is not
allowed.

'master_id' cannot use the simple comments to describe which master it
is. It depends on violation slaves. For example, if there are two
violations:
1. CPU access spi reg
2. CPU access timer reg
It might be different 'master_id' for CPU on these two cases.
I would prefer to remain the id number if translate to a string is a bad
idea.
Thanks !

> 
> >
> > > > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > > +                           vio_dbgs->vio_w.start) == 1;
> > > > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > > +                         vio_dbgs->vio_r.start) == 1;
> > > > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > > +                                 vio_dbgs->addr_h.start;
> > >
> > > What is vio_addr_high? As I know all register address are 32 bits, is
> > > vio_addr_high the address above 32 bits?
> >
> > Yes, you are right. In MT6779, all register base are 32 bits. We can
> > ignore this info for current driver. I'll update on next patch.
> > Thanks !
> 
> Such a strange hardware, all register is 32 bits but it has a
> vio_addr_high in its register. OK, just drop this.
> 
> >
> > >
> > > > +
> > > > +       devapc_vio_info_print(ctx);
> > > > +}
> > > > +
> > >
> > > [snip]
> > >
> > > > +
> > > > +/*
> > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > + *                        violation information including which master violates
> > > > + *                        access slave.
> > > > + */
> > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > +                                       struct mtk_devapc_context *ctx)
> > > > +{
> > > > +       u32 vio_idx;
> > > > +
> > > > +       /*
> > > > +        * Mask slave's irq before clearing vio status.
> > > > +        * Must do it to avoid nested interrupt and prevent
> > > > +        * unexpected behavior.
> > > > +        */
> > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
> > > > +               mask_module_irq(ctx, vio_idx, true);
> > >
> > > I would like to rewrite this for-loop as below to prevent too many
> > > function call in irq handler.
> > >
> > > for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->vio_idx_num); i++)
> > >     writel(0xffffffff, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
> > >
> >
> > This idea is okay for me. Is there any macro to replace 0xffffffff?
> 
> GENMASK(31, 0);
> 
> >
> > > reg  = readl(ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
> > > reg |= 1 << (ctx->vio_idx_num - 32 * i + 1) - 1;
> > > writel(reg, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
> >
> > Are you trying to clear the bits which over vio_idx_num?
> > If yes, I think the second line should be:
> > reg &= 1 << (ctx->vio_idx_num - 32 * i) - 1;
> >
> > For example, if vio_idx_num is 40:
> > after for loop:
> > vio_mask0 = 0xffffffff;
> > vio_mask1 = 0xffffffff;
> 
> when vio_idx_num is 40, VIO_MOD_TO_REG_IND(ctx->vio_idx_num) is 1, so
> after for-loop:
> vio_mask0 = 0xffffffff;
> 
> And the code after for-loop just to do this:
> vio_mask1 = 0xff;
> 
> >
> > reg = readl(vio_mask1);
> > reg &= (1 << 8) - 1; (which is 0x000000ff)
> > reg will be 0xff
> > writel(reg, vio_mask1);
> >
> > Does it make sense?
> >
> > Actually, it is okay to overwrite the unused register bits.
> > So it's no matter to do this step.
> 
> OK, the code would be
> 
> for (i = 0; i <= VIO_MOD_TO_REG_IND(ctx->vio_idx_num - 1); i++)
>     writel(GENMASK(31, 0), ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
> 

Yes, this is okay for me.
Thanks !

> Regards,
> Chun-Kuang.
> 
> >
> > >
> > > > +
> > > > +       devapc_dump_vio_dbg(ctx);
> > > > +
> > > > +       /*
> > > > +        * Ensure that violation info are written
> > > > +        * before further operations
> > > > +        */
> > > > +       smp_mb();
> > > > +
> > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > +               clear_vio_status(ctx, vio_idx);
> > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > +       }
> > >
> > > Ditto for this for-loop.
> >
> > Ditto.
> >
> > >
> > > > +
> > > > +       return IRQ_HANDLED;
> > > > +}
> > > > +
> > >
> > > [snip]
> > >
> > > > +
> > > > +static int mtk_devapc_probe(struct platform_device *pdev)
> > > > +{
> > > > +       struct device_node *node = pdev->dev.of_node;
> > > > +       struct mtk_devapc_context *ctx;
> > > > +       struct clk *devapc_infra_clk;
> > > > +       u32 devapc_irq;
> > > > +       int ret;
> > > > +
> > > > +       if (IS_ERR(node))
> > > > +               return -ENODEV;
> > > > +
> > > > +       ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> > > > +       if (!ctx)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       ctx = (struct mtk_devapc_context *)of_device_get_match_data(&pdev->dev);
> > > > +       ctx->dev = &pdev->dev;
> > > > +
> > > > +       ctx->vio_info = devm_kzalloc(&pdev->dev,
> > > > +                                    sizeof(struct mtk_devapc_vio_info),
> > > > +                                    GFP_KERNEL);
> > > > +       if (!ctx->vio_info)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       ctx->devapc_pd_base = of_iomap(node, 0);
> > > > +       if (!ctx->devapc_pd_base)
> > > > +               return -EINVAL;
> > > > +
> > > > +       devapc_irq = irq_of_parse_and_map(node, 0);
> > > > +       if (!devapc_irq)
> > > > +               return -EINVAL;
> > > > +
> > > > +       devapc_infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> > > > +       if (IS_ERR(devapc_infra_clk))
> > > > +               return -EINVAL;
> > > > +
> > > > +       if (clk_prepare_enable(devapc_infra_clk))
> > > > +               return -EINVAL;
> > > > +
> > > > +       ret = devm_request_irq(&pdev->dev, devapc_irq,
> > > > +                              (irq_handler_t)devapc_violation_irq,
> > > > +                              IRQF_TRIGGER_NONE, "devapc", ctx);
> > > > +       if (ret)
> > >
> > > You should clk_disable_unprepare(devapc_infra_clk);
> >
> > Yes, I miss this part. Thanks for your remind.
> > I'll update it on next patch.
> >
> > >
> > > > +               return ret;
> > > > +
> > > > +       start_devapc(ctx);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int mtk_devapc_remove(struct platform_device *dev)
> > > > +{
> > >
> > > Ditto.
> >
> > Ditto.
> >
> > >
> > > Regards,
> > > Chun-Kuang.
> > >
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static struct platform_driver mtk_devapc_driver = {
> > > > +       .probe = mtk_devapc_probe,
> > > > +       .remove = mtk_devapc_remove,
> > > > +       .driver = {
> > > > +               .name = KBUILD_MODNAME,
> > > > +               .of_match_table = mtk_devapc_dt_match,
> > > > +       },
> > > > +};
> > > > +
> > > > +module_platform_driver(mtk_devapc_driver);
> > > > +
> >

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

Hi Chun-Kuang,

On Fri, 2020-07-31 at 23:55 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年7月31日 週五 上午10:52寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Fri, 2020-07-31 at 00:14 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > > >
> > > > MediaTek bus fabric provides TrustZone security support and data
> > > > protection to prevent slaves from being accessed by unexpected
> > > > masters.
> > > > The security violation is logged and sent to the processor for
> > > > further analysis or countermeasures.
> > > >
> > > > Any occurrence of security violation would raise an interrupt, and
> > > > it will be handled by mtk-devapc driver. The violation
> > > > information is printed in order to find the murderer.
> > > >
> > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > > ---
> > >
> > > [snip]
> > >
> > > > +
> > > > +/*
> > > > + * devapc_extract_vio_dbg - extract full violation information after doing
> > > > + *                          shift mechanism.
> > > > + */
> > > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > > +{
> > > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > +       struct mtk_devapc_vio_info *vio_info;
> > > > +       void __iomem *vio_dbg0_reg;
> > > > +       void __iomem *vio_dbg1_reg;
> > > > +       u32 dbg0;
> > > > +
> > > > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > > +
> > > > +       vio_dbgs = ctx->vio_dbgs;
> > > > +       vio_info = ctx->vio_info;
> > > > +
> > > > +       /* Starts to extract violation information */
> > > > +       dbg0 = readl(vio_dbg0_reg);
> > > > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > > > +
> > > > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > > > +                             vio_dbgs->mstid.start;
> > > > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > > +                             vio_dbgs->dmnid.start;
> > > > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > > +                           vio_dbgs->vio_w.start) == 1;
> > > > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > > +                         vio_dbgs->vio_r.start) == 1;
> > > > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > > +                                 vio_dbgs->addr_h.start;
> > >
> > >
> > > I would like to define the type of ctx->vio_info to be
> > >
> > > struct mtk_devapc_vio_dbgs {
> > >     u32 mstid:16;
> > >     u32 dmnid:6;
> > >     u32 vio_w:1;
> > >     u32 vio_r:1;
> > >     u32 addr_h:4;
> > >     u32 resv:4;
> > > };
> > >
> > > so the code would like the simple way
> > >
> > > ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
> > >
> >
> > This idea looks great! Is there any possible to pass the bit layout by
> > DT data, and still make this operation simple?
> > Why am I asking this question is because this bit layout is platform
> > dependent.
> 
> I doubt these info would be in a single 32-bits register for all
> future SoC. If they are not in single 32-bits register, you may create
> a vio_dbgs_type in DT data, and the code may be
> 
> if (ctx->vio_dbgs_type == VIO_DBGS_TYPE_MTxxxx) {
>     ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
> } else if (ctx->vio_dbgs_type == VIO_DBGS_TYPE_MTyyyy) {
>     ctx->vio_info->mstid = readl(vio_mstid_reg);
>     ctx->vio_info->dmnid = readl(vio_dmnid_reg);
>     ctx->vio_info->vio_w = readl(vio_vio_w_reg);
>     ctx->vio_info->vio_r = readl(vio_vio_r_reg);
> }
> 
> I think we need not to consider how the future would be. Once the
> second SoC driver is upstreaming, we could find out the best solution
> for it.
> 

Okay, I'll apply this on next patch.
Thanks !

> Regards,
> Chun-Kuang.
> 
> >
> > > Regards,
> > > Chun-Kuang.
> > >
> > > > +
> > > > +       devapc_vio_info_print(ctx);
> > > > +}
> > > > +
> >

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

Hi Chun-Kuang,

On Sat, 2020-08-01 at 08:12 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> This patch is for "mediatek,mt6779-devapc", so I think commit title
> should show the SoC ID.

Okay, I'll change title to 'soc:mediatek: add mt6779 devapc driver'.

> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > ---
> 
> [snip]
> 
> > +
> > +struct mtk_devapc_context {
> > +       struct device *dev;
> > +       u32 vio_idx_num;
> > +       void __iomem *devapc_pd_base;
> > +       struct mtk_devapc_vio_info *vio_info;
> > +       const struct mtk_devapc_pd_offset *offset;
> > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > +};
> 
> I think this structure should separate the constant part. The constant part is:
> 
> struct mtk_devapc_data {
>     const u32 vio_idx_num;
>     const struct mtk_devapc_pd_offset *offset; /* I would like to
> remove struct mtk_devapc_pd_offset and directly put its member into
> this structure */
>     const struct mtk_devapc_vio_dbgs *vio_dbgs; /* This may disappear */
> };
> 
> And the context is:
> 
> struct mtk_devapc_context {
>     struct device *dev;
>     void __iomem *devapc_pd_base;
>     const struct mtk_devapc_data *data;
> };
> 
> So when you define this, you would not waste memory to store non-constant data.
> 
> static const struct mtk_devapc_data devapc_mt6779 = {
>  .vio_idx_num = 510,
>  .offset = &mt6779_pd_offset,
>  .vio_dbgs = &mt6779_vio_dbgs,
> };
> 

Sorry, I still don't understand why this refactoring will not waste
memory to store non-constant data. Could you explain more details?
To my understanding, we still also have to allocate memory to store dev
& devapc_pd_base.

> Regards,
> Chun-Kuang.
> 
> > +
> > +#endif /* __MTK_DEVAPC_H__ */
> > --
> > 1.7.9.5
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

Hi Chun-Kuang,

On Sun, 2020-08-02 at 07:50 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > ---
> 
> [snip]
> 
> > +
> > +struct mtk_devapc_context {
> > +       struct device *dev;
> > +       u32 vio_idx_num;
> > +       void __iomem *devapc_pd_base;
> 
> This is devapc context, so prefix 'devapc' is redundant.
> And, what does 'pd' mean?

'pd' means power down. Of course we would also remove it as well.
I suggest to change it as 'infra_base'.

> 
> Regards,
> Chun-Kuang.
> 
> > +       struct mtk_devapc_vio_info *vio_info;
> > +       const struct mtk_devapc_pd_offset *offset;
> > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > +};
> > +

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Neal Liu <neal.liu@mediatek.com> 於 2020年8月3日 週一 下午12:05寫道：
>
> Hi Chun-Kuang,
>
> On Sun, 2020-08-02 at 07:50 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > >
> > > MediaTek bus fabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violation is logged and sent to the processor for
> > > further analysis or countermeasures.
> > >
> > > Any occurrence of security violation would raise an interrupt, and
> > > it will be handled by mtk-devapc driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > ---
> >
> > [snip]
> >
> > > +
> > > +struct mtk_devapc_context {
> > > +       struct device *dev;
> > > +       u32 vio_idx_num;
> > > +       void __iomem *devapc_pd_base;
> >
> > This is devapc context, so prefix 'devapc' is redundant.
> > And, what does 'pd' mean?
>
> 'pd' means power down. Of course we would also remove it as well.
> I suggest to change it as 'infra_base'.

This is OK for me.

>
> >
> > Regards,
> > Chun-Kuang.
> >
> > > +       struct mtk_devapc_vio_info *vio_info;
> > > +       const struct mtk_devapc_pd_offset *offset;
> > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > +};
> > > +
>

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Hi, Neal:

Neal Liu <neal.liu@mediatek.com> 於 2020年8月3日 週一 下午12:01寫道：
>
> Hi Chun-Kuang,
>
> On Sat, 2020-08-01 at 08:12 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > This patch is for "mediatek,mt6779-devapc", so I think commit title
> > should show the SoC ID.
>
> Okay, I'll change title to 'soc:mediatek: add mt6779 devapc driver'.
>
> >
> > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > >
> > > MediaTek bus fabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violation is logged and sent to the processor for
> > > further analysis or countermeasures.
> > >
> > > Any occurrence of security violation would raise an interrupt, and
> > > it will be handled by mtk-devapc driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > ---
> >
> > [snip]
> >
> > > +
> > > +struct mtk_devapc_context {
> > > +       struct device *dev;
> > > +       u32 vio_idx_num;
> > > +       void __iomem *devapc_pd_base;
> > > +       struct mtk_devapc_vio_info *vio_info;
> > > +       const struct mtk_devapc_pd_offset *offset;
> > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > +};
> >
> > I think this structure should separate the constant part. The constant part is:
> >
> > struct mtk_devapc_data {
> >     const u32 vio_idx_num;
> >     const struct mtk_devapc_pd_offset *offset; /* I would like to
> > remove struct mtk_devapc_pd_offset and directly put its member into
> > this structure */
> >     const struct mtk_devapc_vio_dbgs *vio_dbgs; /* This may disappear */
> > };
> >
> > And the context is:
> >
> > struct mtk_devapc_context {
> >     struct device *dev;
> >     void __iomem *devapc_pd_base;
> >     const struct mtk_devapc_data *data;
> > };
> >
> > So when you define this, you would not waste memory to store non-constant data.
> >
> > static const struct mtk_devapc_data devapc_mt6779 = {
> >  .vio_idx_num = 510,
> >  .offset = &mt6779_pd_offset,
> >  .vio_dbgs = &mt6779_vio_dbgs,
> > };
> >
>
> Sorry, I still don't understand why this refactoring will not waste
> memory to store non-constant data. Could you explain more details?
> To my understanding, we still also have to allocate memory to store dev
> & devapc_pd_base.

In some situation, it is. You make the non-constant data a global
variable. I think the context data should be dynamic allocated. If
this driver is not probed, the non-constant data occupy the memory.

Regards,
Chun-Kuang.

>
> > Regards,
> > Chun-Kuang.
> >
> > > +
> > > +#endif /* __MTK_DEVAPC_H__ */
> > > --
> > > 1.7.9.5
> > > _______________________________________________
> > > Linux-mediatek mailing list
> > > Linux-mediatek@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
>

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Hi, Neal:

Neal Liu <neal.liu@mediatek.com> 於 2020年8月3日 週一 上午11:32寫道：
>
> Hi Chun-Kuang,
>
> On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@mediatek.com> 於 2020年7月31日 週五 上午10:44寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > >
> > > On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > > > >
> > > > > MediaTek bus fabric provides TrustZone security support and data
> > > > > protection to prevent slaves from being accessed by unexpected
> > > > > masters.
> > > > > The security violation is logged and sent to the processor for
> > > > > further analysis or countermeasures.
> > > > >
> > > > > Any occurrence of security violation would raise an interrupt, and
> > > > > it will be handled by mtk-devapc driver. The violation
> > > > > information is printed in order to find the murderer.
> > > > >
> > > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > > > ---
> > > >
> > > > [snip]
> > > >
> > > > > +
> > > > > +/*
> > > > > + * devapc_extract_vio_dbg - extract full violation information after doing
> > > > > + *                          shift mechanism.
> > > > > + */
> > > > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > > > +{
> > > > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > > +       struct mtk_devapc_vio_info *vio_info;
> > > > > +       void __iomem *vio_dbg0_reg;
> > > > > +       void __iomem *vio_dbg1_reg;
> > > > > +       u32 dbg0;
> > > > > +
> > > > > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > > > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > > > +
> > > > > +       vio_dbgs = ctx->vio_dbgs;
> > > > > +       vio_info = ctx->vio_info;
> > > > > +
> > > > > +       /* Starts to extract violation information */
> > > > > +       dbg0 = readl(vio_dbg0_reg);
> > > > > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > > > > +
> > > > > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > > > > +                             vio_dbgs->mstid.start;
> > > >
> > > > What is master_id? How could we use it to debug? For example, if we
> > > > get a master_id = 1, what should we do for this?
> > > >
> > > > > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > > > +                             vio_dbgs->dmnid.start;
> > > >
> > > > What is domain_id? How could we use it to debug? For example, if we
> > > > get a domain_id = 2, what should we do for this?
> > > >
> > >
> > > master_id and domain_id belongs our bus side-band signal info. It can
> > > help us to find the violation master.
> >
> > Does 'violation master' means the hardware could access the protected
> > register? (ex. CPU, GCE, ...) If so, I think it's better to add
> > comment to explain how to map (master_id, domain_id) to a hardware
> > (maybe the device in device tree) because every body does not know
> > what the number means. Don't try to translate the number to a string
> > because this would cost much time to do this. Just print a number and
> > we could find out the master by the comment.
>
> 'violation master' means the master which violates the permission
> control. For example, if we set permission 'Secure R/W only' as CPU to
> spi register. When violation is triggered, it means CPU access spi
> register through normal world instead of secure world, which is not
> allowed.
>
> 'master_id' cannot use the simple comments to describe which master it
> is. It depends on violation slaves. For example, if there are two
> violations:
> 1. CPU access spi reg
> 2. CPU access timer reg
> It might be different 'master_id' for CPU on these two cases.
> I would prefer to remain the id number if translate to a string is a bad
> idea.
> Thanks !

It seams that master_id and domain_id does not help for debug. When we
get master_id = 1 and domain_id = 2, we don't know what it mean. I
think we just need violation address because we could find the driver
that write this address and the bug would be inside this driver. So
need not to process master_id and domain_id.

Regards,
Chun-Kuang.

>
> >
> > >
> > > > > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > > > +                           vio_dbgs->vio_w.start) == 1;
> > > > > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > > > +                         vio_dbgs->vio_r.start) == 1;
> > > > > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > > > +                                 vio_dbgs->addr_h.start;
> > > >
> > > > What is vio_addr_high? As I know all register address are 32 bits, is
> > > > vio_addr_high the address above 32 bits?
> > >
> > > Yes, you are right. In MT6779, all register base are 32 bits. We can
> > > ignore this info for current driver. I'll update on next patch.
> > > Thanks !
> >
> > Such a strange hardware, all register is 32 bits but it has a
> > vio_addr_high in its register. OK, just drop this.
> >
> > >
> > > >
> > > > > +
> > > > > +       devapc_vio_info_print(ctx);
> > > > > +}
> > > > > +
> > > >

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

On Tue, 2020-08-04 at 00:04 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年8月3日 週一 下午12:01寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Sat, 2020-08-01 at 08:12 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > This patch is for "mediatek,mt6779-devapc", so I think commit title
> > > should show the SoC ID.
> >
> > Okay, I'll change title to 'soc:mediatek: add mt6779 devapc driver'.
> >
> > >
> > > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > > >
> > > > MediaTek bus fabric provides TrustZone security support and data
> > > > protection to prevent slaves from being accessed by unexpected
> > > > masters.
> > > > The security violation is logged and sent to the processor for
> > > > further analysis or countermeasures.
> > > >
> > > > Any occurrence of security violation would raise an interrupt, and
> > > > it will be handled by mtk-devapc driver. The violation
> > > > information is printed in order to find the murderer.
> > > >
> > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > > ---
> > >
> > > [snip]
> > >
> > > > +
> > > > +struct mtk_devapc_context {
> > > > +       struct device *dev;
> > > > +       u32 vio_idx_num;
> > > > +       void __iomem *devapc_pd_base;
> > > > +       struct mtk_devapc_vio_info *vio_info;
> > > > +       const struct mtk_devapc_pd_offset *offset;
> > > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > +};
> > >
> > > I think this structure should separate the constant part. The constant part is:
> > >
> > > struct mtk_devapc_data {
> > >     const u32 vio_idx_num;
> > >     const struct mtk_devapc_pd_offset *offset; /* I would like to
> > > remove struct mtk_devapc_pd_offset and directly put its member into
> > > this structure */
> > >     const struct mtk_devapc_vio_dbgs *vio_dbgs; /* This may disappear */
> > > };
> > >
> > > And the context is:
> > >
> > > struct mtk_devapc_context {
> > >     struct device *dev;
> > >     void __iomem *devapc_pd_base;
> > >     const struct mtk_devapc_data *data;
> > > };
> > >
> > > So when you define this, you would not waste memory to store non-constant data.
> > >
> > > static const struct mtk_devapc_data devapc_mt6779 = {
> > >  .vio_idx_num = 510,
> > >  .offset = &mt6779_pd_offset,
> > >  .vio_dbgs = &mt6779_vio_dbgs,
> > > };
> > >
> >
> > Sorry, I still don't understand why this refactoring will not waste
> > memory to store non-constant data. Could you explain more details?
> > To my understanding, we still also have to allocate memory to store dev
> > & devapc_pd_base.
> 
> In some situation, it is. You make the non-constant data a global
> variable. I think the context data should be dynamic allocated. If
> this driver is not probed, the non-constant data occupy the memory.
> 

I got your point! In this case, we can save these 2 data structure
space, right?

struct device *dev;
void __iomem *devapc_pd_base;

I'll refactoring this data structures on next patch. Thanks !

> Regards,
> Chun-Kuang.
> 
> >
> > > Regards,
> > > Chun-Kuang.
> > >
> > > > +
> > > > +#endif /* __MTK_DEVAPC_H__ */
> > > > --
> > > > 1.7.9.5
> > > > _______________________________________________
> > > > Linux-mediatek mailing list
> > > > Linux-mediatek@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> >

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Neal Liu]

On Tue, 2020-08-04 at 00:13 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年8月3日 週一 上午11:32寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <neal.liu@mediatek.com> 於 2020年7月31日 週五 上午10:44寫道：
> > > >
> > > > Hi Chun-Kuang,
> > > >
> > > >
> > > > On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > > > > Hi, Neal:
> > > > >
> > > > > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > > > > >
> > > > > > MediaTek bus fabric provides TrustZone security support and data
> > > > > > protection to prevent slaves from being accessed by unexpected
> > > > > > masters.
> > > > > > The security violation is logged and sent to the processor for
> > > > > > further analysis or countermeasures.
> > > > > >
> > > > > > Any occurrence of security violation would raise an interrupt, and
> > > > > > it will be handled by mtk-devapc driver. The violation
> > > > > > information is printed in order to find the murderer.
> > > > > >
> > > > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > > > > ---
> > > > >
> > > > > [snip]
> > > > >
> > > > > > +
> > > > > > +/*
> > > > > > + * devapc_extract_vio_dbg - extract full violation information after doing
> > > > > > + *                          shift mechanism.
> > > > > > + */
> > > > > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > > > > +{
> > > > > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > > > +       struct mtk_devapc_vio_info *vio_info;
> > > > > > +       void __iomem *vio_dbg0_reg;
> > > > > > +       void __iomem *vio_dbg1_reg;
> > > > > > +       u32 dbg0;
> > > > > > +
> > > > > > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > > > > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > > > > +
> > > > > > +       vio_dbgs = ctx->vio_dbgs;
> > > > > > +       vio_info = ctx->vio_info;
> > > > > > +
> > > > > > +       /* Starts to extract violation information */
> > > > > > +       dbg0 = readl(vio_dbg0_reg);
> > > > > > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > > > > > +
> > > > > > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > > > > > +                             vio_dbgs->mstid.start;
> > > > >
> > > > > What is master_id? How could we use it to debug? For example, if we
> > > > > get a master_id = 1, what should we do for this?
> > > > >
> > > > > > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > > > > +                             vio_dbgs->dmnid.start;
> > > > >
> > > > > What is domain_id? How could we use it to debug? For example, if we
> > > > > get a domain_id = 2, what should we do for this?
> > > > >
> > > >
> > > > master_id and domain_id belongs our bus side-band signal info. It can
> > > > help us to find the violation master.
> > >
> > > Does 'violation master' means the hardware could access the protected
> > > register? (ex. CPU, GCE, ...) If so, I think it's better to add
> > > comment to explain how to map (master_id, domain_id) to a hardware
> > > (maybe the device in device tree) because every body does not know
> > > what the number means. Don't try to translate the number to a string
> > > because this would cost much time to do this. Just print a number and
> > > we could find out the master by the comment.
> >
> > 'violation master' means the master which violates the permission
> > control. For example, if we set permission 'Secure R/W only' as CPU to
> > spi register. When violation is triggered, it means CPU access spi
> > register through normal world instead of secure world, which is not
> > allowed.
> >
> > 'master_id' cannot use the simple comments to describe which master it
> > is. It depends on violation slaves. For example, if there are two
> > violations:
> > 1. CPU access spi reg
> > 2. CPU access timer reg
> > It might be different 'master_id' for CPU on these two cases.
> > I would prefer to remain the id number if translate to a string is a bad
> > idea.
> > Thanks !
> 
> It seams that master_id and domain_id does not help for debug. When we
> get master_id = 1 and domain_id = 2, we don't know what it mean. I
> think we just need violation address because we could find the driver
> that write this address and the bug would be inside this driver. So
> need not to process master_id and domain_id.
> 

Actually, it does help us for debug. violation master is not CPU only.
It might be any other master in our SoC. So the bug might not be inside
the kernel driver.
I'll prefer to remain this information.
Thanks !

> Regards,
> Chun-Kuang.
> 
> >
> > >
> > > >
> > > > > > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > > > > +                           vio_dbgs->vio_w.start) == 1;
> > > > > > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > > > > +                         vio_dbgs->vio_r.start) == 1;
> > > > > > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > > > > +                                 vio_dbgs->addr_h.start;
> > > > >
> > > > > What is vio_addr_high? As I know all register address are 32 bits, is
> > > > > vio_addr_high the address above 32 bits?
> > > >
> > > > Yes, you are right. In MT6779, all register base are 32 bits. We can
> > > > ignore this info for current driver. I'll update on next patch.
> > > > Thanks !
> > >
> > > Such a strange hardware, all register is 32 bits but it has a
> > > vio_addr_high in its register. OK, just drop this.
> > >
> > > >
> > > > >
> > > > > > +
> > > > > > +       devapc_vio_info_print(ctx);
> > > > > > +}
> > > > > > +
> > > > >

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Neal Liu <neal.liu@mediatek.com> 於 2020年8月4日 週二 上午10:19寫道：
>
>
> On Tue, 2020-08-04 at 00:13 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@mediatek.com> 於 2020年8月3日 週一 上午11:32寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > > On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > Neal Liu <neal.liu@mediatek.com> 於 2020年7月31日 週五 上午10:44寫道：
> > > > >
> > > > > Hi Chun-Kuang,
> > > > >
> > > > >
> > > > > On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > > > > > Hi, Neal:
> > > > > >
> > > > > > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > > > > > >
> > > > > > > MediaTek bus fabric provides TrustZone security support and data
> > > > > > > protection to prevent slaves from being accessed by unexpected
> > > > > > > masters.
> > > > > > > The security violation is logged and sent to the processor for
> > > > > > > further analysis or countermeasures.
> > > > > > >
> > > > > > > Any occurrence of security violation would raise an interrupt, and
> > > > > > > it will be handled by mtk-devapc driver. The violation
> > > > > > > information is printed in order to find the murderer.
> > > > > > >
> > > > > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > > > > > ---
> > > > > >
> > > > > > [snip]
> > > > > >
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * devapc_extract_vio_dbg - extract full violation information after doing
> > > > > > > + *                          shift mechanism.
> > > > > > > + */
> > > > > > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > > > > > +{
> > > > > > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > > > > +       struct mtk_devapc_vio_info *vio_info;
> > > > > > > +       void __iomem *vio_dbg0_reg;
> > > > > > > +       void __iomem *vio_dbg1_reg;
> > > > > > > +       u32 dbg0;
> > > > > > > +
> > > > > > > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > > > > > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > > > > > +
> > > > > > > +       vio_dbgs = ctx->vio_dbgs;
> > > > > > > +       vio_info = ctx->vio_info;
> > > > > > > +
> > > > > > > +       /* Starts to extract violation information */
> > > > > > > +       dbg0 = readl(vio_dbg0_reg);
> > > > > > > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > > > > > > +
> > > > > > > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > > > > > > +                             vio_dbgs->mstid.start;
> > > > > >
> > > > > > What is master_id? How could we use it to debug? For example, if we
> > > > > > get a master_id = 1, what should we do for this?
> > > > > >
> > > > > > > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > > > > > +                             vio_dbgs->dmnid.start;
> > > > > >
> > > > > > What is domain_id? How could we use it to debug? For example, if we
> > > > > > get a domain_id = 2, what should we do for this?
> > > > > >
> > > > >
> > > > > master_id and domain_id belongs our bus side-band signal info. It can
> > > > > help us to find the violation master.
> > > >
> > > > Does 'violation master' means the hardware could access the protected
> > > > register? (ex. CPU, GCE, ...) If so, I think it's better to add
> > > > comment to explain how to map (master_id, domain_id) to a hardware
> > > > (maybe the device in device tree) because every body does not know
> > > > what the number means. Don't try to translate the number to a string
> > > > because this would cost much time to do this. Just print a number and
> > > > we could find out the master by the comment.
> > >
> > > 'violation master' means the master which violates the permission
> > > control. For example, if we set permission 'Secure R/W only' as CPU to
> > > spi register. When violation is triggered, it means CPU access spi
> > > register through normal world instead of secure world, which is not
> > > allowed.
> > >
> > > 'master_id' cannot use the simple comments to describe which master it
> > > is. It depends on violation slaves. For example, if there are two
> > > violations:
> > > 1. CPU access spi reg
> > > 2. CPU access timer reg
> > > It might be different 'master_id' for CPU on these two cases.
> > > I would prefer to remain the id number if translate to a string is a bad
> > > idea.
> > > Thanks !
> >
> > It seams that master_id and domain_id does not help for debug. When we
> > get master_id = 1 and domain_id = 2, we don't know what it mean. I
> > think we just need violation address because we could find the driver
> > that write this address and the bug would be inside this driver. So
> > need not to process master_id and domain_id.
> >
>
> Actually, it does help us for debug. violation master is not CPU only.
> It might be any other master in our SoC. So the bug might not be inside
> the kernel driver.
> I'll prefer to remain this information.
> Thanks !

Let maintainer to make decision. Maybe he like to print magic number
and wait for someone to hack it.

>
> > Regards,
> > Chun-Kuang.
> >
> > >
> > > >
> > > > >
> > > > > > > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > > > > > +                           vio_dbgs->vio_w.start) == 1;
> > > > > > > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > > > > > +                         vio_dbgs->vio_r.start) == 1;
> > > > > > > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > > > > > +                                 vio_dbgs->addr_h.start;
> > > > > >
> > > > > > What is vio_addr_high? As I know all register address are 32 bits, is
> > > > > > vio_addr_high the address above 32 bits?
> > > > >
> > > > > Yes, you are right. In MT6779, all register base are 32 bits. We can
> > > > > ignore this info for current driver. I'll update on next patch.
> > > > > Thanks !
> > > >
> > > > Such a strange hardware, all register is 32 bits but it has a
> > > > vio_addr_high in its register. OK, just drop this.
> > > >
> > > > >
> > > > > >
> > > > > > > +
> > > > > > > +       devapc_vio_info_print(ctx);
> > > > > > > +}
> > > > > > > +
> > > > > >
>

Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

[Chun-Kuang Hu]

Neal Liu <neal.liu@mediatek.com> 於 2020年8月4日 週二 上午10:08寫道：
>
> On Tue, 2020-08-04 at 00:04 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@mediatek.com> 於 2020年8月3日 週一 下午12:01寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > > On Sat, 2020-08-01 at 08:12 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > This patch is for "mediatek,mt6779-devapc", so I think commit title
> > > > should show the SoC ID.
> > >
> > > Okay, I'll change title to 'soc:mediatek: add mt6779 devapc driver'.
> > >
> > > >
> > > > Neal Liu <neal.liu@mediatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> > > > >
> > > > > MediaTek bus fabric provides TrustZone security support and data
> > > > > protection to prevent slaves from being accessed by unexpected
> > > > > masters.
> > > > > The security violation is logged and sent to the processor for
> > > > > further analysis or countermeasures.
> > > > >
> > > > > Any occurrence of security violation would raise an interrupt, and
> > > > > it will be handled by mtk-devapc driver. The violation
> > > > > information is printed in order to find the murderer.
> > > > >
> > > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > > > ---
> > > >
> > > > [snip]
> > > >
> > > > > +
> > > > > +struct mtk_devapc_context {
> > > > > +       struct device *dev;
> > > > > +       u32 vio_idx_num;
> > > > > +       void __iomem *devapc_pd_base;
> > > > > +       struct mtk_devapc_vio_info *vio_info;
> > > > > +       const struct mtk_devapc_pd_offset *offset;
> > > > > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > > +};
> > > >
> > > > I think this structure should separate the constant part. The constant part is:
> > > >
> > > > struct mtk_devapc_data {
> > > >     const u32 vio_idx_num;
> > > >     const struct mtk_devapc_pd_offset *offset; /* I would like to
> > > > remove struct mtk_devapc_pd_offset and directly put its member into
> > > > this structure */
> > > >     const struct mtk_devapc_vio_dbgs *vio_dbgs; /* This may disappear */
> > > > };
> > > >
> > > > And the context is:
> > > >
> > > > struct mtk_devapc_context {
> > > >     struct device *dev;
> > > >     void __iomem *devapc_pd_base;
> > > >     const struct mtk_devapc_data *data;
> > > > };
> > > >
> > > > So when you define this, you would not waste memory to store non-constant data.
> > > >
> > > > static const struct mtk_devapc_data devapc_mt6779 = {
> > > >  .vio_idx_num = 510,
> > > >  .offset = &mt6779_pd_offset,
> > > >  .vio_dbgs = &mt6779_vio_dbgs,
> > > > };
> > > >
> > >
> > > Sorry, I still don't understand why this refactoring will not waste
> > > memory to store non-constant data. Could you explain more details?
> > > To my understanding, we still also have to allocate memory to store dev
> > > & devapc_pd_base.
> >
> > In some situation, it is. You make the non-constant data a global
> > variable. I think the context data should be dynamic allocated. If
> > this driver is not probed, the non-constant data occupy the memory.
> >
>
> I got your point! In this case, we can save these 2 data structure
> space, right?
>
> struct device *dev;
> void __iomem *devapc_pd_base;

Right.

>
> I'll refactoring this data structures on next patch. Thanks !
>
> > Regards,
> > Chun-Kuang.
> >
> > >
> > > > Regards,
> > > > Chun-Kuang.
> > > >
> > > > > +
> > > > > +#endif /* __MTK_DEVAPC_H__ */
> > > > > --
> > > > > 1.7.9.5
> > > > > _______________________________________________
> > > > > Linux-mediatek mailing list
> > > > > Linux-mediatek@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> > >
>