LinuxLists.cc - [PATCH v4] Add MediaTek MT6779 devapc driver

2020-07-29 08:18:50

Subject: [PATCH v4] Add MediaTek MT6779 devapc driver

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

2020-07-29 08:18:56

by Neal Liu

[permalink] [raw]

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

Add bindings for mtk-devapc.

Signed-off-by: Neal Liu <[email protected]>
---
.../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 <[email protected]>
+
+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

2020-07-29 08:20:52

by Neal Liu

[permalink] [raw]

Subject: [PATCH v4 2/2] soc: mediatek: add mtk-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.

Signed-off-by: Neal Liu <[email protected]>
---
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 <[email protected]>");
+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

2020-07-29 16:39:57

by Chun-Kuang Hu

[permalink] [raw]

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

Hi, Neal:

Neal Liu <[email protected]> 於 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 <[email protected]>
> ---

[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);
> +

2020-07-29 22:49:33

by Chun-Kuang Hu

[permalink] [raw]

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

2020-07-30 16:17:01

by Chun-Kuang Hu

[permalink] [raw]

Neal Liu <[email protected]> 於 2020年8月4日週二上午10:19寫道：
>
>
> On Tue, 2020-08-04 at 00:13 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <[email protected]> 於 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 <[email protected]> 於 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 <[email protected]> 於 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 <[email protected]>
> > > > > > > ---
> > > > > >
> > > > > > [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);
> > > > > > > +}
> > > > > > > +
> > > > > >
>

2020-08-04 15:59:01

by Chun-Kuang Hu

[permalink] [raw]

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

Neal Liu <[email protected]> 於 2020年8月4日週二上午10:08寫道：
>
> On Tue, 2020-08-04 at 00:04 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <[email protected]> 於 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 <[email protected]> 於 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 <[email protected]>
> > > > > ---
> > > >
> > > > [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
> > > > > [email protected]
> > > > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> > >
>