Linux-mediatek Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v7] Add MediaTek MT6779 devapc driver
@ 2020-08-27  3:06 Neal Liu
  2020-08-27  3:06 ` [PATCH v7 1/2] dt-bindings: devapc: add bindings for mtk-devapc Neal Liu
                   ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Neal Liu @ 2020-08-27  3:06 UTC (permalink / raw)
  To: Rob Herring, Matthias Brugger, Chun-Kuang Hu
  Cc: devicetree, wsd_upstream, lkml, linux-mediatek, Neal Liu,
	linux-arm-kernel

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 v6:
- remove unnecessary mask/unmask module irq during ISR.

changes since v5:
- remove redundant write reg operation.
- use static variable of vio_dbgs instead.
- add stop_devapc() if driver is removed.

changes since v4:
- refactor data structure.
- merge two simple functions into one.
- refactor register setting to prevent too many function call overhead.

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 mt6779 devapc driver

 .../bindings/soc/mediatek/devapc.yaml         |  58 ++++
 drivers/soc/mediatek/Kconfig                  |   9 +
 drivers/soc/mediatek/Makefile                 |   1 +
 drivers/soc/mediatek/mtk-devapc.c             | 305 ++++++++++++++++++
 4 files changed, 373 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
 create mode 100644 drivers/soc/mediatek/mtk-devapc.c

-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 1/2] dt-bindings: devapc: add bindings for mtk-devapc
  2020-08-27  3:06 [PATCH v7] Add MediaTek MT6779 devapc driver Neal Liu
@ 2020-08-27  3:06 ` Neal Liu
  2020-08-27  3:06 ` [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver Neal Liu
  2020-09-02  6:40 ` [PATCH v7] Add MediaTek MT6779 " Neal Liu
  2 siblings, 0 replies; 19+ messages in thread
From: Neal Liu @ 2020-08-27  3:06 UTC (permalink / raw)
  To: Rob Herring, Matthias Brugger, Chun-Kuang Hu
  Cc: devicetree, wsd_upstream, lkml, linux-mediatek, Neal Liu,
	linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-08-27  3:06 [PATCH v7] Add MediaTek MT6779 devapc driver Neal Liu
  2020-08-27  3:06 ` [PATCH v7 1/2] dt-bindings: devapc: add bindings for mtk-devapc Neal Liu
@ 2020-08-27  3:06 ` Neal Liu
  2020-10-02 16:24   ` Chun-Kuang Hu
  2020-10-07 10:44   ` Matthias Brugger
  2020-09-02  6:40 ` [PATCH v7] Add MediaTek MT6779 " Neal Liu
  2 siblings, 2 replies; 19+ messages in thread
From: Neal Liu @ 2020-08-27  3:06 UTC (permalink / raw)
  To: Rob Herring, Matthias Brugger, Chun-Kuang Hu
  Cc: devicetree, wsd_upstream, lkml, linux-mediatek, Neal Liu,
	linux-arm-kernel

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 |  305 +++++++++++++++++++++++++++++++++++++
 3 files changed, 315 insertions(+)
 create mode 100644 drivers/soc/mediatek/mtk-devapc.c

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..0ba61d7
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-devapc.c
@@ -0,0 +1,305 @@
+// 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>
+
+#define VIO_MOD_TO_REG_IND(m)	((m) / 32)
+#define VIO_MOD_TO_REG_OFF(m)	((m) % 32)
+
+struct mtk_devapc_vio_dbgs {
+	union {
+		u32 vio_dbg0;
+		struct {
+			u32 mstid:16;
+			u32 dmnid:6;
+			u32 vio_w:1;
+			u32 vio_r:1;
+			u32 addr_h:4;
+			u32 resv:4;
+		} dbg0_bits;
+	};
+
+	u32 vio_dbg1;
+};
+
+struct mtk_devapc_data {
+	u32 vio_idx_num;
+	u32 vio_mask_offset;
+	u32 vio_sta_offset;
+	u32 vio_dbg0_offset;
+	u32 vio_dbg1_offset;
+	u32 apc_con_offset;
+	u32 vio_shift_sta_offset;
+	u32 vio_shift_sel_offset;
+	u32 vio_shift_con_offset;
+};
+
+struct mtk_devapc_context {
+	struct device *dev;
+	void __iomem *infra_base;
+	struct clk *infra_clk;
+	const struct mtk_devapc_data *data;
+};
+
+static void clear_vio_status(struct mtk_devapc_context *ctx)
+{
+	void __iomem *reg;
+	int i;
+
+	reg = ctx->infra_base + ctx->data->vio_sta_offset;
+
+	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
+		writel(GENMASK(31, 0), reg + 4 * i);
+
+	writel(GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1), 0),
+	       reg + 4 * i);
+}
+
+static void mask_module_irq(struct mtk_devapc_context *ctx, bool mask)
+{
+	void __iomem *reg;
+	u32 val;
+	int i;
+
+	reg = ctx->infra_base + ctx->data->vio_mask_offset;
+
+	if (mask)
+		val = GENMASK(31, 0);
+	else
+		val = 0;
+
+	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
+		writel(val, reg + 4 * i);
+
+	val = readl(reg + 4 * i);
+	if (mask)
+		val |= GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
+			       0);
+	else
+		val &= ~GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
+				0);
+
+	writel(val, reg + 4 * i);
+}
+
+#define PHY_DEVAPC_TIMEOUT	0x10000
+
+/*
+ * devapc_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 sync full violation
+ *                       info to VIO_DBGs registers.
+ *
+ */
+static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
+{
+	void __iomem *pd_vio_shift_sta_reg;
+	void __iomem *pd_vio_shift_sel_reg;
+	void __iomem *pd_vio_shift_con_reg;
+	int min_shift_group;
+	int ret;
+	u32 val;
+
+	pd_vio_shift_sta_reg = ctx->infra_base +
+			       ctx->data->vio_shift_sta_offset;
+	pd_vio_shift_sel_reg = ctx->infra_base +
+			       ctx->data->vio_shift_sel_offset;
+	pd_vio_shift_con_reg = ctx->infra_base +
+			       ctx->data->vio_shift_con_offset;
+
+	/* Find the minimum shift group which has violation */
+	val = readl(pd_vio_shift_sta_reg);
+	if (!val)
+		return false;
+
+	min_shift_group = __ffs(val);
+
+	/* Assign the group to sync */
+	writel(0x1 << min_shift_group, pd_vio_shift_sel_reg);
+
+	/* Start syncing */
+	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__);
+		return false;
+	}
+
+	/* Stop syncing */
+	writel(0x0, pd_vio_shift_con_reg);
+
+	/* Write clear */
+	writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);
+
+	return true;
+}
+
+/*
+ * devapc_extract_vio_dbg - extract full violation information after doing
+ *                          shift mechanism.
+ */
+static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
+{
+	struct mtk_devapc_vio_dbgs vio_dbgs;
+	void __iomem *vio_dbg0_reg;
+	void __iomem *vio_dbg1_reg;
+
+	vio_dbg0_reg = ctx->infra_base + ctx->data->vio_dbg0_offset;
+	vio_dbg1_reg = ctx->infra_base + ctx->data->vio_dbg1_offset;
+
+	vio_dbgs.vio_dbg0 = readl(vio_dbg0_reg);
+	vio_dbgs.vio_dbg1 = readl(vio_dbg1_reg);
+
+	/* Print violation information */
+	if (vio_dbgs.dbg0_bits.vio_w)
+		dev_info(ctx->dev, "Write Violation\n");
+	else if (vio_dbgs.dbg0_bits.vio_r)
+		dev_info(ctx->dev, "Read Violation\n");
+
+	dev_info(ctx->dev, "Bus ID:0x%x, Dom ID:0x%x, Vio Addr:0x%x\n",
+		 vio_dbgs.dbg0_bits.mstid, vio_dbgs.dbg0_bits.dmnid,
+		 vio_dbgs.vio_dbg1);
+}
+
+/*
+ * 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)
+{
+	while (devapc_sync_vio_dbg(ctx))
+		devapc_extract_vio_dbg(ctx);
+
+	clear_vio_status(ctx);
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * start_devapc - unmask slave's irq to start receiving devapc violation.
+ */
+static void start_devapc(struct mtk_devapc_context *ctx)
+{
+	writel(BIT(31), ctx->infra_base + ctx->data->apc_con_offset);
+
+	mask_module_irq(ctx, false);
+}
+
+/*
+ * stop_devapc - mask slave's irq to stop service.
+ */
+static void stop_devapc(struct mtk_devapc_context *ctx)
+{
+	mask_module_irq(ctx, true);
+
+	writel(BIT(2), ctx->infra_base + ctx->data->apc_con_offset);
+}
+
+static const struct mtk_devapc_data devapc_mt6779 = {
+	.vio_idx_num = 511,
+	.vio_mask_offset = 0x0,
+	.vio_sta_offset = 0x400,
+	.vio_dbg0_offset = 0x900,
+	.vio_dbg1_offset = 0x904,
+	.apc_con_offset = 0xF00,
+	.vio_shift_sta_offset = 0xF10,
+	.vio_shift_sel_offset = 0xF14,
+	.vio_shift_con_offset = 0xF20,
+};
+
+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;
+	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->data = of_device_get_match_data(&pdev->dev);
+	ctx->dev = &pdev->dev;
+
+	ctx->infra_base = of_iomap(node, 0);
+	if (!ctx->infra_base)
+		return -EINVAL;
+
+	devapc_irq = irq_of_parse_and_map(node, 0);
+	if (!devapc_irq)
+		return -EINVAL;
+
+	ctx->infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
+	if (IS_ERR(ctx->infra_clk))
+		return -EINVAL;
+
+	if (clk_prepare_enable(ctx->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) {
+		clk_disable_unprepare(ctx->infra_clk);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, ctx);
+
+	start_devapc(ctx);
+
+	return 0;
+}
+
+static int mtk_devapc_remove(struct platform_device *pdev)
+{
+	struct mtk_devapc_context *ctx = platform_get_drvdata(pdev);
+
+	stop_devapc(ctx);
+
+	clk_disable_unprepare(ctx->infra_clk);
+
+	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");
-- 
1.7.9.5
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7] Add MediaTek MT6779 devapc driver
  2020-08-27  3:06 [PATCH v7] Add MediaTek MT6779 devapc driver Neal Liu
  2020-08-27  3:06 ` [PATCH v7 1/2] dt-bindings: devapc: add bindings for mtk-devapc Neal Liu
  2020-08-27  3:06 ` [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver Neal Liu
@ 2020-09-02  6:40 ` Neal Liu
  2020-09-09  8:37   ` Neal Liu
  2 siblings, 1 reply; 19+ messages in thread
From: Neal Liu @ 2020-09-02  6:40 UTC (permalink / raw)
  To: Rob Herring, Chun-Kuang Hu, Matthias Brugger
  Cc: devicetree, wsd_upstream, lkml, linux-mediatek, Neal Liu,
	linux-arm-kernel

Hi Rob, Matthias, Chun-Kuang,

Gentle ping for this patch set.
Thanks

-Neal

On Thu, 2020-08-27 at 11:06 +0800, Neal Liu wrote:
> 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 v6:
> - remove unnecessary mask/unmask module irq during ISR.
> 
> changes since v5:
> - remove redundant write reg operation.
> - use static variable of vio_dbgs instead.
> - add stop_devapc() if driver is removed.
> 
> changes since v4:
> - refactor data structure.
> - merge two simple functions into one.
> - refactor register setting to prevent too many function call overhead.
> 
> 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 mt6779 devapc driver
> 
>  .../bindings/soc/mediatek/devapc.yaml         |  58 ++++
>  drivers/soc/mediatek/Kconfig                  |   9 +
>  drivers/soc/mediatek/Makefile                 |   1 +
>  drivers/soc/mediatek/mtk-devapc.c             | 305 ++++++++++++++++++
>  4 files changed, 373 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
>  create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7] Add MediaTek MT6779 devapc driver
  2020-09-02  6:40 ` [PATCH v7] Add MediaTek MT6779 " Neal Liu
@ 2020-09-09  8:37   ` Neal Liu
  2020-09-16  8:58     ` Neal Liu
  0 siblings, 1 reply; 19+ messages in thread
From: Neal Liu @ 2020-09-09  8:37 UTC (permalink / raw)
  To: Rob Herring, Matthias Brugger, Chun-Kuang Hu
  Cc: devicetree, wsd_upstream, lkml, linux-mediatek, Neal Liu,
	linux-arm-kernel

Hi Rob, Matthias, Chun-Kuang,

Please kindly let me know your comments about this patch set.
Thanks

-Neal

On Wed, 2020-09-02 at 14:40 +0800, Neal Liu wrote:
> Hi Rob, Matthias, Chun-Kuang,
> 
> Gentle ping for this patch set.
> Thanks
> 
> -Neal
> 
> On Thu, 2020-08-27 at 11:06 +0800, Neal Liu wrote:
> > 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 v6:
> > - remove unnecessary mask/unmask module irq during ISR.
> > 
> > changes since v5:
> > - remove redundant write reg operation.
> > - use static variable of vio_dbgs instead.
> > - add stop_devapc() if driver is removed.
> > 
> > changes since v4:
> > - refactor data structure.
> > - merge two simple functions into one.
> > - refactor register setting to prevent too many function call overhead.
> > 
> > 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 mt6779 devapc driver
> > 
> >  .../bindings/soc/mediatek/devapc.yaml         |  58 ++++
> >  drivers/soc/mediatek/Kconfig                  |   9 +
> >  drivers/soc/mediatek/Makefile                 |   1 +
> >  drivers/soc/mediatek/mtk-devapc.c             | 305 ++++++++++++++++++
> >  4 files changed, 373 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
> >  create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> > 
> 
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7] Add MediaTek MT6779 devapc driver
  2020-09-09  8:37   ` Neal Liu
@ 2020-09-16  8:58     ` Neal Liu
  2020-09-22  7:13       ` Neal Liu
  0 siblings, 1 reply; 19+ messages in thread
From: Neal Liu @ 2020-09-16  8:58 UTC (permalink / raw)
  To: Rob Herring, Chun-Kuang Hu, Matthias Brugger
  Cc: devicetree, wsd_upstream, lkml, linux-mediatek, Neal Liu,
	linux-arm-kernel

Hi Rob, Matthias, Chun-Kuang,

Sorry for pushing you so hard.
May I know is this patch set is comfortable to apply on latest kernel?
Thanks

-Neal

On Wed, 2020-09-09 at 16:37 +0800, Neal Liu wrote:
> Hi Rob, Matthias, Chun-Kuang,
> 
> Please kindly let me know your comments about this patch set.
> Thanks
> 
> -Neal
> 
> On Wed, 2020-09-02 at 14:40 +0800, Neal Liu wrote:
> > Hi Rob, Matthias, Chun-Kuang,
> > 
> > Gentle ping for this patch set.
> > Thanks
> > 
> > -Neal
> > 
> > On Thu, 2020-08-27 at 11:06 +0800, Neal Liu wrote:
> > > 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 v6:
> > > - remove unnecessary mask/unmask module irq during ISR.
> > > 
> > > changes since v5:
> > > - remove redundant write reg operation.
> > > - use static variable of vio_dbgs instead.
> > > - add stop_devapc() if driver is removed.
> > > 
> > > changes since v4:
> > > - refactor data structure.
> > > - merge two simple functions into one.
> > > - refactor register setting to prevent too many function call overhead.
> > > 
> > > 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 mt6779 devapc driver
> > > 
> > >  .../bindings/soc/mediatek/devapc.yaml         |  58 ++++
> > >  drivers/soc/mediatek/Kconfig                  |   9 +
> > >  drivers/soc/mediatek/Makefile                 |   1 +
> > >  drivers/soc/mediatek/mtk-devapc.c             | 305 ++++++++++++++++++
> > >  4 files changed, 373 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
> > >  create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> > > 
> > 
> > 
> 
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7] Add MediaTek MT6779 devapc driver
  2020-09-16  8:58     ` Neal Liu
@ 2020-09-22  7:13       ` Neal Liu
  2020-09-30  7:10         ` Neal Liu
  0 siblings, 1 reply; 19+ messages in thread
From: Neal Liu @ 2020-09-22  7:13 UTC (permalink / raw)
  To: Matthias Brugger, Rob Herring
  Cc: Chun-Kuang Hu, wsd_upstream, devicetree, lkml, linux-mediatek,
	Neal Liu, linux-arm-kernel

Hi Matthias,

We need this driver supported on main-line.
Could you save your time for us to review it?
Thanks

-Neal

On Wed, 2020-09-16 at 16:58 +0800, Neal Liu wrote:
> Hi Rob, Matthias, Chun-Kuang,
> 
> Sorry for pushing you so hard.
> May I know is this patch set is comfortable to apply on latest kernel?
> Thanks
> 
> -Neal
> 
> On Wed, 2020-09-09 at 16:37 +0800, Neal Liu wrote:
> > Hi Rob, Matthias, Chun-Kuang,
> > 
> > Please kindly let me know your comments about this patch set.
> > Thanks
> > 
> > -Neal
> > 
> > On Wed, 2020-09-02 at 14:40 +0800, Neal Liu wrote:
> > > Hi Rob, Matthias, Chun-Kuang,
> > > 
> > > Gentle ping for this patch set.
> > > Thanks
> > > 
> > > -Neal
> > > 
> > > On Thu, 2020-08-27 at 11:06 +0800, Neal Liu wrote:
> > > > 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 v6:
> > > > - remove unnecessary mask/unmask module irq during ISR.
> > > > 
> > > > changes since v5:
> > > > - remove redundant write reg operation.
> > > > - use static variable of vio_dbgs instead.
> > > > - add stop_devapc() if driver is removed.
> > > > 
> > > > changes since v4:
> > > > - refactor data structure.
> > > > - merge two simple functions into one.
> > > > - refactor register setting to prevent too many function call overhead.
> > > > 
> > > > 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 mt6779 devapc driver
> > > > 
> > > >  .../bindings/soc/mediatek/devapc.yaml         |  58 ++++
> > > >  drivers/soc/mediatek/Kconfig                  |   9 +
> > > >  drivers/soc/mediatek/Makefile                 |   1 +
> > > >  drivers/soc/mediatek/mtk-devapc.c             | 305 ++++++++++++++++++
> > > >  4 files changed, 373 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
> > > >  create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> > > > 
> > > 
> > > 
> > 
> > 
> 
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7] Add MediaTek MT6779 devapc driver
  2020-09-22  7:13       ` Neal Liu
@ 2020-09-30  7:10         ` Neal Liu
  2020-10-02 16:17           ` Chun-Kuang Hu
  0 siblings, 1 reply; 19+ messages in thread
From: Neal Liu @ 2020-09-30  7:10 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: Chun-Kuang Hu, wsd_upstream, devicetree, lkml, Rob Herring,
	linux-mediatek, Neal Liu, linux-arm-kernel

Hi Matt,

Hope this mail could find you well.
Is everything okay?
It would be glad if you could reply me no matter the review status.

Thanks

-Neal

On Tue, 2020-09-22 at 15:13 +0800, Neal Liu wrote:
> Hi Matthias,
> 
> We need this driver supported on main-line.
> Could you save your time for us to review it?
> Thanks
> 
> -Neal
> 
> On Wed, 2020-09-16 at 16:58 +0800, Neal Liu wrote:
> > Hi Rob, Matthias, Chun-Kuang,
> > 
> > Sorry for pushing you so hard.
> > May I know is this patch set is comfortable to apply on latest kernel?
> > Thanks
> > 
> > -Neal
> > 
> > On Wed, 2020-09-09 at 16:37 +0800, Neal Liu wrote:
> > > Hi Rob, Matthias, Chun-Kuang,
> > > 
> > > Please kindly let me know your comments about this patch set.
> > > Thanks
> > > 
> > > -Neal
> > > 
> > > On Wed, 2020-09-02 at 14:40 +0800, Neal Liu wrote:
> > > > Hi Rob, Matthias, Chun-Kuang,
> > > > 
> > > > Gentle ping for this patch set.
> > > > Thanks
> > > > 
> > > > -Neal
> > > > 
> > > > On Thu, 2020-08-27 at 11:06 +0800, Neal Liu wrote:
> > > > > 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 v6:
> > > > > - remove unnecessary mask/unmask module irq during ISR.
> > > > > 
> > > > > changes since v5:
> > > > > - remove redundant write reg operation.
> > > > > - use static variable of vio_dbgs instead.
> > > > > - add stop_devapc() if driver is removed.
> > > > > 
> > > > > changes since v4:
> > > > > - refactor data structure.
> > > > > - merge two simple functions into one.
> > > > > - refactor register setting to prevent too many function call overhead.
> > > > > 
> > > > > 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 mt6779 devapc driver
> > > > > 
> > > > >  .../bindings/soc/mediatek/devapc.yaml         |  58 ++++
> > > > >  drivers/soc/mediatek/Kconfig                  |   9 +
> > > > >  drivers/soc/mediatek/Makefile                 |   1 +
> > > > >  drivers/soc/mediatek/mtk-devapc.c             | 305 ++++++++++++++++++
> > > > >  4 files changed, 373 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
> > > > >  create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> > > > > 
> > > > 
> > > > 
> > > 
> > > 
> > 
> > 
> 
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7] Add MediaTek MT6779 devapc driver
  2020-09-30  7:10         ` Neal Liu
@ 2020-10-02 16:17           ` Chun-Kuang Hu
  0 siblings, 0 replies; 19+ messages in thread
From: Chun-Kuang Hu @ 2020-10-02 16:17 UTC (permalink / raw)
  To: Neal Liu
  Cc: Chun-Kuang Hu, wsd_upstream, devicetree, lkml, Rob Herring,
	linux-mediatek, Matthias Brugger, linux-arm-kernel

Hi, Neal:

You may find Matthias in IRC [1], the channel name is #linux-mediatek

[1] https://webchat.freenode.net/

Neal Liu <neal.liu@mediatek.com> 於 2020年9月30日 週三 下午3:10寫道:
>
> Hi Matt,
>
> Hope this mail could find you well.
> Is everything okay?
> It would be glad if you could reply me no matter the review status.
>
> Thanks
>
> -Neal
>
> On Tue, 2020-09-22 at 15:13 +0800, Neal Liu wrote:
> > Hi Matthias,
> >
> > We need this driver supported on main-line.
> > Could you save your time for us to review it?
> > Thanks
> >
> > -Neal
> >
> > On Wed, 2020-09-16 at 16:58 +0800, Neal Liu wrote:
> > > Hi Rob, Matthias, Chun-Kuang,
> > >
> > > Sorry for pushing you so hard.
> > > May I know is this patch set is comfortable to apply on latest kernel?
> > > Thanks
> > >
> > > -Neal
> > >
> > > On Wed, 2020-09-09 at 16:37 +0800, Neal Liu wrote:
> > > > Hi Rob, Matthias, Chun-Kuang,
> > > >
> > > > Please kindly let me know your comments about this patch set.
> > > > Thanks
> > > >
> > > > -Neal
> > > >
> > > > On Wed, 2020-09-02 at 14:40 +0800, Neal Liu wrote:
> > > > > Hi Rob, Matthias, Chun-Kuang,
> > > > >
> > > > > Gentle ping for this patch set.
> > > > > Thanks
> > > > >
> > > > > -Neal
> > > > >
> > > > > On Thu, 2020-08-27 at 11:06 +0800, Neal Liu wrote:
> > > > > > 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 v6:
> > > > > > - remove unnecessary mask/unmask module irq during ISR.
> > > > > >
> > > > > > changes since v5:
> > > > > > - remove redundant write reg operation.
> > > > > > - use static variable of vio_dbgs instead.
> > > > > > - add stop_devapc() if driver is removed.
> > > > > >
> > > > > > changes since v4:
> > > > > > - refactor data structure.
> > > > > > - merge two simple functions into one.
> > > > > > - refactor register setting to prevent too many function call overhead.
> > > > > >
> > > > > > 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 mt6779 devapc driver
> > > > > >
> > > > > >  .../bindings/soc/mediatek/devapc.yaml         |  58 ++++
> > > > > >  drivers/soc/mediatek/Kconfig                  |   9 +
> > > > > >  drivers/soc/mediatek/Makefile                 |   1 +
> > > > > >  drivers/soc/mediatek/mtk-devapc.c             | 305 ++++++++++++++++++
> > > > > >  4 files changed, 373 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc.yaml
> > > > > >  create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> > > > > >
> > > > >
> > > > >
> > > >
> > > >
> > >
> > >
> >
> >
>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-08-27  3:06 ` [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver Neal Liu
@ 2020-10-02 16:24   ` Chun-Kuang Hu
  2020-10-05  7:20     ` Neal Liu
  2020-10-07 10:44   ` Matthias Brugger
  1 sibling, 1 reply; 19+ messages in thread
From: Chun-Kuang Hu @ 2020-10-02 16:24 UTC (permalink / raw)
  To: Neal Liu
  Cc: Chun-Kuang Hu, wsd_upstream, DTML, lkml, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Matthias Brugger,
	Linux ARM

Hi, Neal:

Neal Liu <neal.liu@mediatek.com> 於 2020年8月27日 週四 上午11:07寫道:
>
> 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 |  305 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 315 insertions(+)
>  create mode 100644 drivers/soc/mediatek/mtk-devapc.c
>

[snip]

> +
> +static int mtk_devapc_probe(struct platform_device *pdev)
> +{
> +       struct device_node *node = pdev->dev.of_node;
> +       struct mtk_devapc_context *ctx;
> +       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->data = of_device_get_match_data(&pdev->dev);
> +       ctx->dev = &pdev->dev;
> +
> +       ctx->infra_base = of_iomap(node, 0);
> +       if (!ctx->infra_base)
> +               return -EINVAL;
> +
> +       devapc_irq = irq_of_parse_and_map(node, 0);
> +       if (!devapc_irq)
> +               return -EINVAL;
> +
> +       ctx->infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> +       if (IS_ERR(ctx->infra_clk))
> +               return -EINVAL;
> +
> +       if (clk_prepare_enable(ctx->infra_clk))
> +               return -EINVAL;

What would happen if you do not enable this clock? I think this
hardware is already initialized in trust zone.

Regards,
Chun-Kuang.

> +
> +       ret = devm_request_irq(&pdev->dev, devapc_irq,
> +                              (irq_handler_t)devapc_violation_irq,
> +                              IRQF_TRIGGER_NONE, "devapc", ctx);
> +       if (ret) {
> +               clk_disable_unprepare(ctx->infra_clk);
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, ctx);
> +
> +       start_devapc(ctx);
> +
> +       return 0;
> +}
> +

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-10-02 16:24   ` Chun-Kuang Hu
@ 2020-10-05  7:20     ` Neal Liu
  0 siblings, 0 replies; 19+ messages in thread
From: Neal Liu @ 2020-10-05  7:20 UTC (permalink / raw)
  To: Chun-Kuang Hu
  Cc: DTML, wsd_upstream, lkml, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Matthias Brugger,
	Neal Liu, Linux ARM

Hi Chun-Kuang,

On Sat, 2020-10-03 at 00:24 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年8月27日 週四 上午11:07寫道:
> >
> > 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 |  305 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 315 insertions(+)
> >  create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> >
> 
> [snip]
> 
> > +
> > +static int mtk_devapc_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *node = pdev->dev.of_node;
> > +       struct mtk_devapc_context *ctx;
> > +       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->data = of_device_get_match_data(&pdev->dev);
> > +       ctx->dev = &pdev->dev;
> > +
> > +       ctx->infra_base = of_iomap(node, 0);
> > +       if (!ctx->infra_base)
> > +               return -EINVAL;
> > +
> > +       devapc_irq = irq_of_parse_and_map(node, 0);
> > +       if (!devapc_irq)
> > +               return -EINVAL;
> > +
> > +       ctx->infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> > +       if (IS_ERR(ctx->infra_clk))
> > +               return -EINVAL;
> > +
> > +       if (clk_prepare_enable(ctx->infra_clk))
> > +               return -EINVAL;
> 
> What would happen if you do not enable this clock? I think this
> hardware is already initialized in trust zone.
> 
> Regards,
> Chun-Kuang.

It cannot handle violation if the clock is disabled.
This parts of hardware is not initialized in TrustZone.

The another parts of hardware is initialized in TrustZone which is
responsible for permission control. I think that is the part what you
intend to express.

-Neal

> 
> > +
> > +       ret = devm_request_irq(&pdev->dev, devapc_irq,
> > +                              (irq_handler_t)devapc_violation_irq,
> > +                              IRQF_TRIGGER_NONE, "devapc", ctx);
> > +       if (ret) {
> > +               clk_disable_unprepare(ctx->infra_clk);
> > +               return ret;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, ctx);
> > +
> > +       start_devapc(ctx);
> > +
> > +       return 0;
> > +}
> > +

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-08-27  3:06 ` [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver Neal Liu
  2020-10-02 16:24   ` Chun-Kuang Hu
@ 2020-10-07 10:44   ` Matthias Brugger
  2020-10-08  2:35     ` Neal Liu
  1 sibling, 1 reply; 19+ messages in thread
From: Matthias Brugger @ 2020-10-07 10:44 UTC (permalink / raw)
  To: Neal Liu, Rob Herring, Chun-Kuang Hu
  Cc: devicetree, linux-mediatek, lkml, linux-arm-kernel, wsd_upstream



On 27/08/2020 05:06, Neal Liu wrote:
> 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.

"The violation information is printed in order to find the responsible component."

Nobody got actually killed, right :)

> 
> 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 |  305 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 315 insertions(+)
>   create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> 
> 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..0ba61d7
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-devapc.c
> @@ -0,0 +1,305 @@
> +// 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>
> +
> +#define VIO_MOD_TO_REG_IND(m)	((m) / 32)
> +#define VIO_MOD_TO_REG_OFF(m)	((m) % 32)
> +
> +struct mtk_devapc_vio_dbgs {
> +	union {
> +		u32 vio_dbg0;
> +		struct {
> +			u32 mstid:16;
> +			u32 dmnid:6;
> +			u32 vio_w:1;
> +			u32 vio_r:1;
> +			u32 addr_h:4;
> +			u32 resv:4;
> +		} dbg0_bits;
> +	};
> +
> +	u32 vio_dbg1;
> +};
> +
> +struct mtk_devapc_data {
> +	u32 vio_idx_num;
> +	u32 vio_mask_offset;
> +	u32 vio_sta_offset;
> +	u32 vio_dbg0_offset;
> +	u32 vio_dbg1_offset;
> +	u32 apc_con_offset;
> +	u32 vio_shift_sta_offset;
> +	u32 vio_shift_sel_offset;
> +	u32 vio_shift_con_offset;
> +};

Please describe the fields of the struct, that will make it easier to understand 
the driver.

> +
> +struct mtk_devapc_context {
> +	struct device *dev;
> +	void __iomem *infra_base;
> +	struct clk *infra_clk;
> +	const struct mtk_devapc_data *data;
> +};
> +
> +static void clear_vio_status(struct mtk_devapc_context *ctx)
> +{
> +	void __iomem *reg;
> +	int i;
> +
> +	reg = ctx->infra_base + ctx->data->vio_sta_offset;
> +
> +	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
> +		writel(GENMASK(31, 0), reg + 4 * i);
> +
> +	writel(GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1), 0),
> +	       reg + 4 * i);
> +}
> +
> +static void mask_module_irq(struct mtk_devapc_context *ctx, bool mask)
> +{
> +	void __iomem *reg;
> +	u32 val;
> +	int i;
> +
> +	reg = ctx->infra_base + ctx->data->vio_mask_offset;
> +
> +	if (mask)
> +		val = GENMASK(31, 0);
> +	else
> +		val = 0;
> +
> +	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)

Do I get that right? We have a number of virtual IO identifier. Their 
correspondending interrupt are grouped in 32 bit registers. And we want to 
enable/disable them by writing 0 or 1. We have to take care of the last 
registers as it could be the case that vio_idx_num is not a multiple of 32, correct?

In this case we should traverse VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1 
registers, which is (vio_idx_num / 32) - 1 and not (vio_idx_num - 1) / 32.

> +		writel(val, reg + 4 * i);
> +
> +	val = readl(reg + 4 * i);
> +	if (mask)
> +		val |= GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
> +			       0);

We have 511 IRQs, which gives us 31 bits in the last register to set/unset. 
Thats 510..0 bits, so from what I understand, once again we want
GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1, 0)
which is (vio_idx_num % 32) - 1

Correct or do I understand something wrong?
If so, same applies to clear_vio_status().


> +	else
> +		val &= ~GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
> +				0);
> +
> +	writel(val, reg + 4 * i);
> +}
> +
> +#define PHY_DEVAPC_TIMEOUT	0x10000
> +
> +/*
> + * devapc_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 sync full violation
> + *                       info to VIO_DBGs registers.
> + *
> + */
> +static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
> +{
> +	void __iomem *pd_vio_shift_sta_reg;
> +	void __iomem *pd_vio_shift_sel_reg;
> +	void __iomem *pd_vio_shift_con_reg;
> +	int min_shift_group;
> +	int ret;
> +	u32 val;
> +
> +	pd_vio_shift_sta_reg = ctx->infra_base +
> +			       ctx->data->vio_shift_sta_offset;
> +	pd_vio_shift_sel_reg = ctx->infra_base +
> +			       ctx->data->vio_shift_sel_offset;
> +	pd_vio_shift_con_reg = ctx->infra_base +
> +			       ctx->data->vio_shift_con_offset;
> +
> +	/* Find the minimum shift group which has violation */
> +	val = readl(pd_vio_shift_sta_reg);
> +	if (!val)
> +		return false;

So bit 0 of selection register (pd_vio_shift_sel_reg) does not represent a 
violation group?
I don't know how the HW works, but is seems odd to me. In case that's bit 0 
actually doesn't represent anything: how can an interrupt be triggered without 
any debug information present (means val == 0)?

> +
> +	min_shift_group = __ffs(val);
> +
> +	/* Assign the group to sync */
> +	writel(0x1 << min_shift_group, pd_vio_shift_sel_reg);
> +
> +	/* Start syncing */
> +	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__);

In which case this can happen? I'm asking, because we are calling 
devapc_sync_vio_dbg() in a while loop that could make the kernel hang here.

Do I understand correctly, that we are using the while loop, because there can 
be more then one violation group which got triggered (read, more then one bit is 
set in pd_vio_shift_sta_reg)? Would it make more sense then to read the register 
once and do all the shift operation for all groups which bit set to 1 in the 
shift status register?

> +		return false;
> +	}
> +
> +	/* Stop syncing */
> +	writel(0x0, pd_vio_shift_con_reg);
> +
> +	/* Write clear */
> +	writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);
> +
> +	return true;
> +}
> +
> +/*
> + * devapc_extract_vio_dbg - extract full violation information after doing
> + *                          shift mechanism.
> + */
> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> +{
> +	struct mtk_devapc_vio_dbgs vio_dbgs;
> +	void __iomem *vio_dbg0_reg;
> +	void __iomem *vio_dbg1_reg;
> +
> +	vio_dbg0_reg = ctx->infra_base + ctx->data->vio_dbg0_offset;
> +	vio_dbg1_reg = ctx->infra_base + ctx->data->vio_dbg1_offset;
> +
> +	vio_dbgs.vio_dbg0 = readl(vio_dbg0_reg);
> +	vio_dbgs.vio_dbg1 = readl(vio_dbg1_reg);
> +
> +	/* Print violation information */
> +	if (vio_dbgs.dbg0_bits.vio_w)
> +		dev_info(ctx->dev, "Write Violation\n");
> +	else if (vio_dbgs.dbg0_bits.vio_r)
> +		dev_info(ctx->dev, "Read Violation\n");
> +
> +	dev_info(ctx->dev, "Bus ID:0x%x, Dom ID:0x%x, Vio Addr:0x%x\n",
> +		 vio_dbgs.dbg0_bits.mstid, vio_dbgs.dbg0_bits.dmnid,
> +		 vio_dbgs.vio_dbg1);
> +}
> +
> +/*
> + * 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)

static irqreturn_t devapc_violation_irq(int irq_number, void *data)
{
	struct mtk_devapc_context *ctx = data;

> +{
> +	while (devapc_sync_vio_dbg(ctx))
> +		devapc_extract_vio_dbg(ctx);
> +
> +	clear_vio_status(ctx);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * start_devapc - unmask slave's irq to start receiving devapc violation.
> + */
> +static void start_devapc(struct mtk_devapc_context *ctx)
> +{
> +	writel(BIT(31), ctx->infra_base + ctx->data->apc_con_offset);
> +
> +	mask_module_irq(ctx, false);
> +}
> +
> +/*
> + * stop_devapc - mask slave's irq to stop service.
> + */
> +static void stop_devapc(struct mtk_devapc_context *ctx)
> +{
> +	mask_module_irq(ctx, true);
> +
> +	writel(BIT(2), ctx->infra_base + ctx->data->apc_con_offset);
> +}
> +
> +static const struct mtk_devapc_data devapc_mt6779 = {
> +	.vio_idx_num = 511,
> +	.vio_mask_offset = 0x0,
> +	.vio_sta_offset = 0x400,
> +	.vio_dbg0_offset = 0x900,
> +	.vio_dbg1_offset = 0x904,
> +	.apc_con_offset = 0xF00,
> +	.vio_shift_sta_offset = 0xF10,
> +	.vio_shift_sel_offset = 0xF14,
> +	.vio_shift_con_offset = 0xF20,
> +};
> +
> +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;
> +	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->data = of_device_get_match_data(&pdev->dev);
> +	ctx->dev = &pdev->dev;
> +
> +	ctx->infra_base = of_iomap(node, 0);

Does this mean the device is part of the infracfg block?
I wasn't able to find any information about it.

> +	if (!ctx->infra_base)
> +		return -EINVAL;
> +
> +	devapc_irq = irq_of_parse_and_map(node, 0);
> +	if (!devapc_irq)
> +		return -EINVAL;
> +
> +	ctx->infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> +	if (IS_ERR(ctx->infra_clk))
> +		return -EINVAL;
> +
> +	if (clk_prepare_enable(ctx->infra_clk))
> +		return -EINVAL;
> +
> +	ret = devm_request_irq(&pdev->dev, devapc_irq,
> +			       (irq_handler_t)devapc_violation_irq,

No cast should be needed.

> +			       IRQF_TRIGGER_NONE, "devapc", ctx);
> +	if (ret) {
> +		clk_disable_unprepare(ctx->infra_clk);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, ctx);
> +
> +	start_devapc(ctx);
> +
> +	return 0;
> +}
> +
> +static int mtk_devapc_remove(struct platform_device *pdev)
> +{
> +	struct mtk_devapc_context *ctx = platform_get_drvdata(pdev);
> +
> +	stop_devapc(ctx);
> +
> +	clk_disable_unprepare(ctx->infra_clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mtk_devapc_driver = {
> +	.probe = mtk_devapc_probe,
> +	.remove = mtk_devapc_remove,
> +	.driver = {
> +		.name = KBUILD_MODNAME,

.name = "mtk-devapc",

Regards,
Matthias

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-10-07 10:44   ` Matthias Brugger
@ 2020-10-08  2:35     ` Neal Liu
  2020-10-08  8:45       ` Matthias Brugger
  2020-10-15  2:13       ` Neal Liu
  0 siblings, 2 replies; 19+ messages in thread
From: Neal Liu @ 2020-10-08  2:35 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: Chun-Kuang Hu, wsd_upstream, devicetree, lkml, Rob Herring,
	linux-mediatek, Neal Liu, linux-arm-kernel

On Wed, 2020-10-07 at 12:44 +0200, Matthias Brugger wrote:
> 
> On 27/08/2020 05:06, Neal Liu wrote:
> > 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.
> 
> "The violation information is printed in order to find the responsible component."
> 
> Nobody got actually killed, right :)

Correct !
> 
> > 
> > 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 |  305 +++++++++++++++++++++++++++++++++++++
> >   3 files changed, 315 insertions(+)
> >   create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> > 
> > 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..0ba61d7
> > --- /dev/null
> > +++ b/drivers/soc/mediatek/mtk-devapc.c
> > @@ -0,0 +1,305 @@
> > +// 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>
> > +
> > +#define VIO_MOD_TO_REG_IND(m)	((m) / 32)
> > +#define VIO_MOD_TO_REG_OFF(m)	((m) % 32)
> > +
> > +struct mtk_devapc_vio_dbgs {
> > +	union {
> > +		u32 vio_dbg0;
> > +		struct {
> > +			u32 mstid:16;
> > +			u32 dmnid:6;
> > +			u32 vio_w:1;
> > +			u32 vio_r:1;
> > +			u32 addr_h:4;
> > +			u32 resv:4;
> > +		} dbg0_bits;
> > +	};
> > +
> > +	u32 vio_dbg1;
> > +};
> > +
> > +struct mtk_devapc_data {
> > +	u32 vio_idx_num;
> > +	u32 vio_mask_offset;
> > +	u32 vio_sta_offset;
> > +	u32 vio_dbg0_offset;
> > +	u32 vio_dbg1_offset;
> > +	u32 apc_con_offset;
> > +	u32 vio_shift_sta_offset;
> > +	u32 vio_shift_sel_offset;
> > +	u32 vio_shift_con_offset;
> > +};
> 
> Please describe the fields of the struct, that will make it easier to understand 
> the driver.

Okay, I'll try to add more description about this struct. May be like:

struct mtk_devapc_data {
	/* numbers of violation index */
	u32 vio_idx_num;

	/* reg offset */
	u32 vio_mask_offset;
	u32 vio_sta_offset;
	u32 vio_dbg0_offset;
	u32 vio_dbg1_offset;
	u32 apc_con_offset;
	u32 vio_shift_sta_offset;
	u32 vio_shift_sel_offset;
	u32 vio_shift_con_offset;
};

> 
> > +
> > +struct mtk_devapc_context {
> > +	struct device *dev;
> > +	void __iomem *infra_base;
> > +	struct clk *infra_clk;
> > +	const struct mtk_devapc_data *data;
> > +};
> > +
> > +static void clear_vio_status(struct mtk_devapc_context *ctx)
> > +{
> > +	void __iomem *reg;
> > +	int i;
> > +
> > +	reg = ctx->infra_base + ctx->data->vio_sta_offset;
> > +
> > +	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
> > +		writel(GENMASK(31, 0), reg + 4 * i);
> > +
> > +	writel(GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1), 0),
> > +	       reg + 4 * i);
> > +}
> > +
> > +static void mask_module_irq(struct mtk_devapc_context *ctx, bool mask)
> > +{
> > +	void __iomem *reg;
> > +	u32 val;
> > +	int i;
> > +
> > +	reg = ctx->infra_base + ctx->data->vio_mask_offset;
> > +
> > +	if (mask)
> > +		val = GENMASK(31, 0);
> > +	else
> > +		val = 0;
> > +
> > +	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
> 
> Do I get that right? We have a number of virtual IO identifier. Their 
> correspondending interrupt are grouped in 32 bit registers. And we want to 
> enable/disable them by writing 0 or 1. We have to take care of the last 
> registers as it could be the case that vio_idx_num is not a multiple of 32, correct?
> 
> In this case we should traverse VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1 
> registers, which is (vio_idx_num / 32) - 1 and not (vio_idx_num - 1) / 32.
> 

Yes, your understanding is correct. It should be
VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1 instead of
VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1).

> > +		writel(val, reg + 4 * i);
> > +
> > +	val = readl(reg + 4 * i);
> > +	if (mask)
> > +		val |= GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
> > +			       0);
> 
> We have 511 IRQs, which gives us 31 bits in the last register to set/unset. 
> Thats 510..0 bits, so from what I understand, once again we want
> GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1, 0)
> which is (vio_idx_num % 32) - 1
> 
> Correct or do I understand something wrong?
> If so, same applies to clear_vio_status().
> 

Correct. I'll fix it on next patch.
Thanks

> 
> > +	else
> > +		val &= ~GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
> > +				0);
> > +
> > +	writel(val, reg + 4 * i);
> > +}
> > +
> > +#define PHY_DEVAPC_TIMEOUT	0x10000
> > +
> > +/*
> > + * devapc_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 sync full violation
> > + *                       info to VIO_DBGs registers.
> > + *
> > + */
> > +static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > +	void __iomem *pd_vio_shift_sta_reg;
> > +	void __iomem *pd_vio_shift_sel_reg;
> > +	void __iomem *pd_vio_shift_con_reg;
> > +	int min_shift_group;
> > +	int ret;
> > +	u32 val;
> > +
> > +	pd_vio_shift_sta_reg = ctx->infra_base +
> > +			       ctx->data->vio_shift_sta_offset;
> > +	pd_vio_shift_sel_reg = ctx->infra_base +
> > +			       ctx->data->vio_shift_sel_offset;
> > +	pd_vio_shift_con_reg = ctx->infra_base +
> > +			       ctx->data->vio_shift_con_offset;
> > +
> > +	/* Find the minimum shift group which has violation */
> > +	val = readl(pd_vio_shift_sta_reg);
> > +	if (!val)
> > +		return false;
> 
> So bit 0 of selection register (pd_vio_shift_sel_reg) does not represent a 
> violation group?
> I don't know how the HW works, but is seems odd to me. In case that's bit 0 
> actually doesn't represent anything: how can an interrupt be triggered without 
> any debug information present (means val == 0)?

This check implies HW status has something wrong. It cannot get any
debug information for this case.
It won't happen in normal scenario. Should we remove this check?

> 
> > +
> > +	min_shift_group = __ffs(val);
> > +
> > +	/* Assign the group to sync */
> > +	writel(0x1 << min_shift_group, pd_vio_shift_sel_reg);
> > +
> > +	/* Start syncing */
> > +	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__);
> 
> In which case this can happen? I'm asking, because we are calling 
> devapc_sync_vio_dbg() in a while loop that could make the kernel hang here.
> 
> Do I understand correctly, that we are using the while loop, because there can 
> be more then one violation group which got triggered (read, more then one bit is 
> set in pd_vio_shift_sta_reg)? Would it make more sense then to read the register 
> once and do all the shift operation for all groups which bit set to 1 in the 
> shift status register?

Yes, your understanding is correct.
This check also implies HW status has something wrong. We return false
to skip further violation info dump.
How could this case make the kernel hang?

> 
> > +		return false;
> > +	}
> > +
> > +	/* Stop syncing */
> > +	writel(0x0, pd_vio_shift_con_reg);
> > +
> > +	/* Write clear */
> > +	writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * devapc_extract_vio_dbg - extract full violation information after doing
> > + *                          shift mechanism.
> > + */
> > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > +	struct mtk_devapc_vio_dbgs vio_dbgs;
> > +	void __iomem *vio_dbg0_reg;
> > +	void __iomem *vio_dbg1_reg;
> > +
> > +	vio_dbg0_reg = ctx->infra_base + ctx->data->vio_dbg0_offset;
> > +	vio_dbg1_reg = ctx->infra_base + ctx->data->vio_dbg1_offset;
> > +
> > +	vio_dbgs.vio_dbg0 = readl(vio_dbg0_reg);
> > +	vio_dbgs.vio_dbg1 = readl(vio_dbg1_reg);
> > +
> > +	/* Print violation information */
> > +	if (vio_dbgs.dbg0_bits.vio_w)
> > +		dev_info(ctx->dev, "Write Violation\n");
> > +	else if (vio_dbgs.dbg0_bits.vio_r)
> > +		dev_info(ctx->dev, "Read Violation\n");
> > +
> > +	dev_info(ctx->dev, "Bus ID:0x%x, Dom ID:0x%x, Vio Addr:0x%x\n",
> > +		 vio_dbgs.dbg0_bits.mstid, vio_dbgs.dbg0_bits.dmnid,
> > +		 vio_dbgs.vio_dbg1);
> > +}
> > +
> > +/*
> > + * 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)
> 
> static irqreturn_t devapc_violation_irq(int irq_number, void *data)
> {
> 	struct mtk_devapc_context *ctx = data;

Okay, I'll fix it on next patch.
Thanks

> 
> > +{
> > +	while (devapc_sync_vio_dbg(ctx))
> > +		devapc_extract_vio_dbg(ctx);
> > +
> > +	clear_vio_status(ctx);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * start_devapc - unmask slave's irq to start receiving devapc violation.
> > + */
> > +static void start_devapc(struct mtk_devapc_context *ctx)
> > +{
> > +	writel(BIT(31), ctx->infra_base + ctx->data->apc_con_offset);
> > +
> > +	mask_module_irq(ctx, false);
> > +}
> > +
> > +/*
> > + * stop_devapc - mask slave's irq to stop service.
> > + */
> > +static void stop_devapc(struct mtk_devapc_context *ctx)
> > +{
> > +	mask_module_irq(ctx, true);
> > +
> > +	writel(BIT(2), ctx->infra_base + ctx->data->apc_con_offset);
> > +}
> > +
> > +static const struct mtk_devapc_data devapc_mt6779 = {
> > +	.vio_idx_num = 511,
> > +	.vio_mask_offset = 0x0,
> > +	.vio_sta_offset = 0x400,
> > +	.vio_dbg0_offset = 0x900,
> > +	.vio_dbg1_offset = 0x904,
> > +	.apc_con_offset = 0xF00,
> > +	.vio_shift_sta_offset = 0xF10,
> > +	.vio_shift_sel_offset = 0xF14,
> > +	.vio_shift_con_offset = 0xF20,
> > +};
> > +
> > +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;
> > +	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->data = of_device_get_match_data(&pdev->dev);
> > +	ctx->dev = &pdev->dev;
> > +
> > +	ctx->infra_base = of_iomap(node, 0);
> 
> Does this mean the device is part of the infracfg block?
> I wasn't able to find any information about it.

I'm not sure why you would ask infracfg block. devapc is parts of our
SoC infra, it's different with infracfg.

> 
> > +	if (!ctx->infra_base)
> > +		return -EINVAL;
> > +
> > +	devapc_irq = irq_of_parse_and_map(node, 0);
> > +	if (!devapc_irq)
> > +		return -EINVAL;
> > +
> > +	ctx->infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> > +	if (IS_ERR(ctx->infra_clk))
> > +		return -EINVAL;
> > +
> > +	if (clk_prepare_enable(ctx->infra_clk))
> > +		return -EINVAL;
> > +
> > +	ret = devm_request_irq(&pdev->dev, devapc_irq,
> > +			       (irq_handler_t)devapc_violation_irq,
> 
> No cast should be needed.

Okay, I'll remove it on next patch.
Thanks

> 
> > +			       IRQF_TRIGGER_NONE, "devapc", ctx);
> > +	if (ret) {
> > +		clk_disable_unprepare(ctx->infra_clk);
> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, ctx);
> > +
> > +	start_devapc(ctx);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_devapc_remove(struct platform_device *pdev)
> > +{
> > +	struct mtk_devapc_context *ctx = platform_get_drvdata(pdev);
> > +
> > +	stop_devapc(ctx);
> > +
> > +	clk_disable_unprepare(ctx->infra_clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver mtk_devapc_driver = {
> > +	.probe = mtk_devapc_probe,
> > +	.remove = mtk_devapc_remove,
> > +	.driver = {
> > +		.name = KBUILD_MODNAME,
> 
> .name = "mtk-devapc",

Okay, I'll add it on next patch.
Thanks

> 
> Regards,
> Matthias

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-10-08  2:35     ` Neal Liu
@ 2020-10-08  8:45       ` Matthias Brugger
  2020-10-08  9:39         ` Neal Liu
  2020-10-15  2:13       ` Neal Liu
  1 sibling, 1 reply; 19+ messages in thread
From: Matthias Brugger @ 2020-10-08  8:45 UTC (permalink / raw)
  To: Neal Liu
  Cc: Chun-Kuang Hu, wsd_upstream, devicetree, lkml, Rob Herring,
	linux-mediatek, linux-arm-kernel



On 08/10/2020 04:35, Neal Liu wrote:
> On Wed, 2020-10-07 at 12:44 +0200, Matthias Brugger wrote:
>>
>> On 27/08/2020 05:06, Neal Liu wrote:
>>> 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.
>>
>> "The violation information is printed in order to find the responsible component."
>>
>> Nobody got actually killed, right :)
> 
> Correct !
>>
>>>
>>> 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 |  305 +++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 315 insertions(+)
>>>    create mode 100644 drivers/soc/mediatek/mtk-devapc.c
>>>
>>> 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..0ba61d7
>>> --- /dev/null
>>> +++ b/drivers/soc/mediatek/mtk-devapc.c
>>> @@ -0,0 +1,305 @@
>>> +// 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>
>>> +
>>> +#define VIO_MOD_TO_REG_IND(m)	((m) / 32)
>>> +#define VIO_MOD_TO_REG_OFF(m)	((m) % 32)
>>> +
>>> +struct mtk_devapc_vio_dbgs {
>>> +	union {
>>> +		u32 vio_dbg0;
>>> +		struct {
>>> +			u32 mstid:16;
>>> +			u32 dmnid:6;
>>> +			u32 vio_w:1;
>>> +			u32 vio_r:1;
>>> +			u32 addr_h:4;
>>> +			u32 resv:4;
>>> +		} dbg0_bits;
>>> +	};
>>> +
>>> +	u32 vio_dbg1;
>>> +};
>>> +
>>> +struct mtk_devapc_data {
>>> +	u32 vio_idx_num;
>>> +	u32 vio_mask_offset;
>>> +	u32 vio_sta_offset;
>>> +	u32 vio_dbg0_offset;
>>> +	u32 vio_dbg1_offset;
>>> +	u32 apc_con_offset;
>>> +	u32 vio_shift_sta_offset;
>>> +	u32 vio_shift_sel_offset;
>>> +	u32 vio_shift_con_offset;
>>> +};
>>
>> Please describe the fields of the struct, that will make it easier to understand
>> the driver.
> 
> Okay, I'll try to add more description about this struct. May be like:
> 
> struct mtk_devapc_data {
> 	/* numbers of violation index */
> 	u32 vio_idx_num;
> 
> 	/* reg offset */
> 	u32 vio_mask_offset;
> 	u32 vio_sta_offset;
> 	u32 vio_dbg0_offset;
> 	u32 vio_dbg1_offset;
> 	u32 apc_con_offset;
> 	u32 vio_shift_sta_offset;
> 	u32 vio_shift_sel_offset;
> 	u32 vio_shift_con_offset;
> };
> 
>>
>>> +
>>> +struct mtk_devapc_context {
>>> +	struct device *dev;
>>> +	void __iomem *infra_base;
>>> +	struct clk *infra_clk;
>>> +	const struct mtk_devapc_data *data;
>>> +};
>>> +
>>> +static void clear_vio_status(struct mtk_devapc_context *ctx)
>>> +{
>>> +	void __iomem *reg;
>>> +	int i;
>>> +
>>> +	reg = ctx->infra_base + ctx->data->vio_sta_offset;
>>> +
>>> +	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
>>> +		writel(GENMASK(31, 0), reg + 4 * i);
>>> +
>>> +	writel(GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1), 0),
>>> +	       reg + 4 * i);
>>> +}
>>> +
>>> +static void mask_module_irq(struct mtk_devapc_context *ctx, bool mask)
>>> +{
>>> +	void __iomem *reg;
>>> +	u32 val;
>>> +	int i;
>>> +
>>> +	reg = ctx->infra_base + ctx->data->vio_mask_offset;
>>> +
>>> +	if (mask)
>>> +		val = GENMASK(31, 0);
>>> +	else
>>> +		val = 0;
>>> +
>>> +	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
>>
>> Do I get that right? We have a number of virtual IO identifier. Their
>> correspondending interrupt are grouped in 32 bit registers. And we want to
>> enable/disable them by writing 0 or 1. We have to take care of the last
>> registers as it could be the case that vio_idx_num is not a multiple of 32, correct?
>>
>> In this case we should traverse VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1
>> registers, which is (vio_idx_num / 32) - 1 and not (vio_idx_num - 1) / 32.
>>
> 
> Yes, your understanding is correct. It should be
> VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1 instead of
> VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1).
> 
>>> +		writel(val, reg + 4 * i);
>>> +
>>> +	val = readl(reg + 4 * i);
>>> +	if (mask)
>>> +		val |= GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
>>> +			       0);
>>
>> We have 511 IRQs, which gives us 31 bits in the last register to set/unset.
>> Thats 510..0 bits, so from what I understand, once again we want
>> GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1, 0)
>> which is (vio_idx_num % 32) - 1
>>
>> Correct or do I understand something wrong?
>> If so, same applies to clear_vio_status().
>>
> 
> Correct. I'll fix it on next patch.
> Thanks
> 
>>
>>> +	else
>>> +		val &= ~GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
>>> +				0);
>>> +
>>> +	writel(val, reg + 4 * i);
>>> +}
>>> +
>>> +#define PHY_DEVAPC_TIMEOUT	0x10000
>>> +
>>> +/*
>>> + * devapc_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 sync full violation
>>> + *                       info to VIO_DBGs registers.
>>> + *
>>> + */
>>> +static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
>>> +{
>>> +	void __iomem *pd_vio_shift_sta_reg;
>>> +	void __iomem *pd_vio_shift_sel_reg;
>>> +	void __iomem *pd_vio_shift_con_reg;
>>> +	int min_shift_group;
>>> +	int ret;
>>> +	u32 val;
>>> +
>>> +	pd_vio_shift_sta_reg = ctx->infra_base +
>>> +			       ctx->data->vio_shift_sta_offset;
>>> +	pd_vio_shift_sel_reg = ctx->infra_base +
>>> +			       ctx->data->vio_shift_sel_offset;
>>> +	pd_vio_shift_con_reg = ctx->infra_base +
>>> +			       ctx->data->vio_shift_con_offset;
>>> +
>>> +	/* Find the minimum shift group which has violation */
>>> +	val = readl(pd_vio_shift_sta_reg);
>>> +	if (!val)
>>> +		return false;
>>
>> So bit 0 of selection register (pd_vio_shift_sel_reg) does not represent a
>> violation group?
>> I don't know how the HW works, but is seems odd to me. In case that's bit 0
>> actually doesn't represent anything: how can an interrupt be triggered without
>> any debug information present (means val == 0)?
> 
> This check implies HW status has something wrong. It cannot get any
> debug information for this case.
> It won't happen in normal scenario. Should we remove this check?
> 

No I think the check is fine. I'd add a WARN() message as this is an indicator 
that the HW is not working corretly.

>>
>>> +
>>> +	min_shift_group = __ffs(val);
>>> +
>>> +	/* Assign the group to sync */
>>> +	writel(0x1 << min_shift_group, pd_vio_shift_sel_reg);
>>> +
>>> +	/* Start syncing */
>>> +	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__);
>>
>> In which case this can happen? I'm asking, because we are calling
>> devapc_sync_vio_dbg() in a while loop that could make the kernel hang here.
>>
>> Do I understand correctly, that we are using the while loop, because there can
>> be more then one violation group which got triggered (read, more then one bit is
>> set in pd_vio_shift_sta_reg)? Would it make more sense then to read the register
>> once and do all the shift operation for all groups which bit set to 1 in the
>> shift status register?
> 
> Yes, your understanding is correct.
> This check also implies HW status has something wrong. We return false
> to skip further violation info dump.
> How could this case make the kernel hang?
> 

The kernel can't hang, sorry my fault. In any case I'd add a WARN() here as 
well. I understand that if we error out here, we have a HW/FW bug (my feeling 
is, that behind this is a micro controller of some kind :)

>>
>>> +		return false;
>>> +	}
>>> +
>>> +	/* Stop syncing */
>>> +	writel(0x0, pd_vio_shift_con_reg);
>>> +
>>> +	/* Write clear */
>>> +	writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +/*
>>> + * devapc_extract_vio_dbg - extract full violation information after doing
>>> + *                          shift mechanism.
>>> + */
>>> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
>>> +{
>>> +	struct mtk_devapc_vio_dbgs vio_dbgs;
>>> +	void __iomem *vio_dbg0_reg;
>>> +	void __iomem *vio_dbg1_reg;
>>> +
>>> +	vio_dbg0_reg = ctx->infra_base + ctx->data->vio_dbg0_offset;
>>> +	vio_dbg1_reg = ctx->infra_base + ctx->data->vio_dbg1_offset;
>>> +
>>> +	vio_dbgs.vio_dbg0 = readl(vio_dbg0_reg);
>>> +	vio_dbgs.vio_dbg1 = readl(vio_dbg1_reg);
>>> +
>>> +	/* Print violation information */
>>> +	if (vio_dbgs.dbg0_bits.vio_w)
>>> +		dev_info(ctx->dev, "Write Violation\n");
>>> +	else if (vio_dbgs.dbg0_bits.vio_r)
>>> +		dev_info(ctx->dev, "Read Violation\n");
>>> +
>>> +	dev_info(ctx->dev, "Bus ID:0x%x, Dom ID:0x%x, Vio Addr:0x%x\n",
>>> +		 vio_dbgs.dbg0_bits.mstid, vio_dbgs.dbg0_bits.dmnid,
>>> +		 vio_dbgs.vio_dbg1);
>>> +}
>>> +
>>> +/*
>>> + * 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)
>>
>> static irqreturn_t devapc_violation_irq(int irq_number, void *data)
>> {
>> 	struct mtk_devapc_context *ctx = data;
> 
> Okay, I'll fix it on next patch.
> Thanks
> 
>>
>>> +{
>>> +	while (devapc_sync_vio_dbg(ctx))
>>> +		devapc_extract_vio_dbg(ctx);
>>> +
>>> +	clear_vio_status(ctx);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +/*
>>> + * start_devapc - unmask slave's irq to start receiving devapc violation.
>>> + */
>>> +static void start_devapc(struct mtk_devapc_context *ctx)
>>> +{
>>> +	writel(BIT(31), ctx->infra_base + ctx->data->apc_con_offset);
>>> +
>>> +	mask_module_irq(ctx, false);
>>> +}
>>> +
>>> +/*
>>> + * stop_devapc - mask slave's irq to stop service.
>>> + */
>>> +static void stop_devapc(struct mtk_devapc_context *ctx)
>>> +{
>>> +	mask_module_irq(ctx, true);
>>> +
>>> +	writel(BIT(2), ctx->infra_base + ctx->data->apc_con_offset);
>>> +}
>>> +
>>> +static const struct mtk_devapc_data devapc_mt6779 = {
>>> +	.vio_idx_num = 511,
>>> +	.vio_mask_offset = 0x0,
>>> +	.vio_sta_offset = 0x400,
>>> +	.vio_dbg0_offset = 0x900,
>>> +	.vio_dbg1_offset = 0x904,
>>> +	.apc_con_offset = 0xF00,
>>> +	.vio_shift_sta_offset = 0xF10,
>>> +	.vio_shift_sel_offset = 0xF14,
>>> +	.vio_shift_con_offset = 0xF20,
>>> +};
>>> +
>>> +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;
>>> +	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->data = of_device_get_match_data(&pdev->dev);
>>> +	ctx->dev = &pdev->dev;
>>> +
>>> +	ctx->infra_base = of_iomap(node, 0);
>>
>> Does this mean the device is part of the infracfg block?
>> I wasn't able to find any information about it.
> 
> I'm not sure why you would ask infracfg block. devapc is parts of our
> SoC infra, it's different with infracfg.
> 

I'm asking because I want to understand the HW better. I'm not able to find any 
information in the datasheets. I want to avoid a situation as we had with the 
MMSYS where a clock driver was submitted first and later on we realized that 
MMSYS is much more then that and we had to work hard to get the driver right.

Now it's happening with SCPSYS, where a driver with the scpsys compatible was 
send years ago. But SCPSYS is much more then the driver submitted. In this case 
we opted to write a new driver, but moving from one driver to another one is 
painfull and full of problems. For that I want to make sure we fully understand 
Device APC (by the way, what does APC stands for?). Is it a totally independent 
HW block or is it part of a subsystem, like for example SCP?

Regards,
Matthias

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-10-08  8:45       ` Matthias Brugger
@ 2020-10-08  9:39         ` Neal Liu
  2020-10-09 12:34           ` Matthias Brugger
  0 siblings, 1 reply; 19+ messages in thread
From: Neal Liu @ 2020-10-08  9:39 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: Chun-Kuang Hu, wsd_upstream, devicetree, lkml, Rob Herring,
	linux-mediatek, Neal Liu, linux-arm-kernel

On Thu, 2020-10-08 at 10:45 +0200, Matthias Brugger wrote:
> 
> On 08/10/2020 04:35, Neal Liu wrote:
> > On Wed, 2020-10-07 at 12:44 +0200, Matthias Brugger wrote:
> >>
> >> On 27/08/2020 05:06, Neal Liu wrote:
> >>> 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.
> >>
> >> "The violation information is printed in order to find the responsible component."
> >>
> >> Nobody got actually killed, right :)
> > 
> > Correct !
> >>
> >>>
> >>> 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 |  305 +++++++++++++++++++++++++++++++++++++
> >>>    3 files changed, 315 insertions(+)
> >>>    create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> >>>
> >>> 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..0ba61d7
> >>> --- /dev/null
> >>> +++ b/drivers/soc/mediatek/mtk-devapc.c
> >>> @@ -0,0 +1,305 @@
> >>> +// 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>
> >>> +
> >>> +#define VIO_MOD_TO_REG_IND(m)	((m) / 32)
> >>> +#define VIO_MOD_TO_REG_OFF(m)	((m) % 32)
> >>> +
> >>> +struct mtk_devapc_vio_dbgs {
> >>> +	union {
> >>> +		u32 vio_dbg0;
> >>> +		struct {
> >>> +			u32 mstid:16;
> >>> +			u32 dmnid:6;
> >>> +			u32 vio_w:1;
> >>> +			u32 vio_r:1;
> >>> +			u32 addr_h:4;
> >>> +			u32 resv:4;
> >>> +		} dbg0_bits;
> >>> +	};
> >>> +
> >>> +	u32 vio_dbg1;
> >>> +};
> >>> +
> >>> +struct mtk_devapc_data {
> >>> +	u32 vio_idx_num;
> >>> +	u32 vio_mask_offset;
> >>> +	u32 vio_sta_offset;
> >>> +	u32 vio_dbg0_offset;
> >>> +	u32 vio_dbg1_offset;
> >>> +	u32 apc_con_offset;
> >>> +	u32 vio_shift_sta_offset;
> >>> +	u32 vio_shift_sel_offset;
> >>> +	u32 vio_shift_con_offset;
> >>> +};
> >>
> >> Please describe the fields of the struct, that will make it easier to understand
> >> the driver.
> > 
> > Okay, I'll try to add more description about this struct. May be like:
> > 
> > struct mtk_devapc_data {
> > 	/* numbers of violation index */
> > 	u32 vio_idx_num;
> > 
> > 	/* reg offset */
> > 	u32 vio_mask_offset;
> > 	u32 vio_sta_offset;
> > 	u32 vio_dbg0_offset;
> > 	u32 vio_dbg1_offset;
> > 	u32 apc_con_offset;
> > 	u32 vio_shift_sta_offset;
> > 	u32 vio_shift_sel_offset;
> > 	u32 vio_shift_con_offset;
> > };
> > 
> >>
> >>> +
> >>> +struct mtk_devapc_context {
> >>> +	struct device *dev;
> >>> +	void __iomem *infra_base;
> >>> +	struct clk *infra_clk;
> >>> +	const struct mtk_devapc_data *data;
> >>> +};
> >>> +
> >>> +static void clear_vio_status(struct mtk_devapc_context *ctx)
> >>> +{
> >>> +	void __iomem *reg;
> >>> +	int i;
> >>> +
> >>> +	reg = ctx->infra_base + ctx->data->vio_sta_offset;
> >>> +
> >>> +	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
> >>> +		writel(GENMASK(31, 0), reg + 4 * i);
> >>> +
> >>> +	writel(GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1), 0),
> >>> +	       reg + 4 * i);
> >>> +}
> >>> +
> >>> +static void mask_module_irq(struct mtk_devapc_context *ctx, bool mask)
> >>> +{
> >>> +	void __iomem *reg;
> >>> +	u32 val;
> >>> +	int i;
> >>> +
> >>> +	reg = ctx->infra_base + ctx->data->vio_mask_offset;
> >>> +
> >>> +	if (mask)
> >>> +		val = GENMASK(31, 0);
> >>> +	else
> >>> +		val = 0;
> >>> +
> >>> +	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
> >>
> >> Do I get that right? We have a number of virtual IO identifier. Their
> >> correspondending interrupt are grouped in 32 bit registers. And we want to
> >> enable/disable them by writing 0 or 1. We have to take care of the last
> >> registers as it could be the case that vio_idx_num is not a multiple of 32, correct?
> >>
> >> In this case we should traverse VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1
> >> registers, which is (vio_idx_num / 32) - 1 and not (vio_idx_num - 1) / 32.
> >>
> > 
> > Yes, your understanding is correct. It should be
> > VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1 instead of
> > VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1).
> > 
> >>> +		writel(val, reg + 4 * i);
> >>> +
> >>> +	val = readl(reg + 4 * i);
> >>> +	if (mask)
> >>> +		val |= GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
> >>> +			       0);
> >>
> >> We have 511 IRQs, which gives us 31 bits in the last register to set/unset.
> >> Thats 510..0 bits, so from what I understand, once again we want
> >> GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1, 0)
> >> which is (vio_idx_num % 32) - 1
> >>
> >> Correct or do I understand something wrong?
> >> If so, same applies to clear_vio_status().
> >>
> > 
> > Correct. I'll fix it on next patch.
> > Thanks
> > 
> >>
> >>> +	else
> >>> +		val &= ~GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
> >>> +				0);
> >>> +
> >>> +	writel(val, reg + 4 * i);
> >>> +}
> >>> +
> >>> +#define PHY_DEVAPC_TIMEOUT	0x10000
> >>> +
> >>> +/*
> >>> + * devapc_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 sync full violation
> >>> + *                       info to VIO_DBGs registers.
> >>> + *
> >>> + */
> >>> +static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
> >>> +{
> >>> +	void __iomem *pd_vio_shift_sta_reg;
> >>> +	void __iomem *pd_vio_shift_sel_reg;
> >>> +	void __iomem *pd_vio_shift_con_reg;
> >>> +	int min_shift_group;
> >>> +	int ret;
> >>> +	u32 val;
> >>> +
> >>> +	pd_vio_shift_sta_reg = ctx->infra_base +
> >>> +			       ctx->data->vio_shift_sta_offset;
> >>> +	pd_vio_shift_sel_reg = ctx->infra_base +
> >>> +			       ctx->data->vio_shift_sel_offset;
> >>> +	pd_vio_shift_con_reg = ctx->infra_base +
> >>> +			       ctx->data->vio_shift_con_offset;
> >>> +
> >>> +	/* Find the minimum shift group which has violation */
> >>> +	val = readl(pd_vio_shift_sta_reg);
> >>> +	if (!val)
> >>> +		return false;
> >>
> >> So bit 0 of selection register (pd_vio_shift_sel_reg) does not represent a
> >> violation group?
> >> I don't know how the HW works, but is seems odd to me. In case that's bit 0
> >> actually doesn't represent anything: how can an interrupt be triggered without
> >> any debug information present (means val == 0)?
> > 
> > This check implies HW status has something wrong. It cannot get any
> > debug information for this case.
> > It won't happen in normal scenario. Should we remove this check?
> > 
> 
> No I think the check is fine. I'd add a WARN() message as this is an indicator 
> that the HW is not working corretly.

Sure. add WARN() message is more helpful to understand.
I'll add it on next patch.
Thanks

> 
> >>
> >>> +
> >>> +	min_shift_group = __ffs(val);
> >>> +
> >>> +	/* Assign the group to sync */
> >>> +	writel(0x1 << min_shift_group, pd_vio_shift_sel_reg);
> >>> +
> >>> +	/* Start syncing */
> >>> +	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__);
> >>
> >> In which case this can happen? I'm asking, because we are calling
> >> devapc_sync_vio_dbg() in a while loop that could make the kernel hang here.
> >>
> >> Do I understand correctly, that we are using the while loop, because there can
> >> be more then one violation group which got triggered (read, more then one bit is
> >> set in pd_vio_shift_sta_reg)? Would it make more sense then to read the register
> >> once and do all the shift operation for all groups which bit set to 1 in the
> >> shift status register?
> > 
> > Yes, your understanding is correct.
> > This check also implies HW status has something wrong. We return false
> > to skip further violation info dump.
> > How could this case make the kernel hang?
> > 
> 
> The kernel can't hang, sorry my fault. In any case I'd add a WARN() here as 
> well. I understand that if we error out here, we have a HW/FW bug (my feeling 
> is, that behind this is a micro controller of some kind :)
> 

I'll also add warn messages on next patch.

> >>
> >>> +		return false;
> >>> +	}
> >>> +
> >>> +	/* Stop syncing */
> >>> +	writel(0x0, pd_vio_shift_con_reg);
> >>> +
> >>> +	/* Write clear */
> >>> +	writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);
> >>> +
> >>> +	return true;
> >>> +}
> >>> +
> >>> +/*
> >>> + * devapc_extract_vio_dbg - extract full violation information after doing
> >>> + *                          shift mechanism.
> >>> + */
> >>> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> >>> +{
> >>> +	struct mtk_devapc_vio_dbgs vio_dbgs;
> >>> +	void __iomem *vio_dbg0_reg;
> >>> +	void __iomem *vio_dbg1_reg;
> >>> +
> >>> +	vio_dbg0_reg = ctx->infra_base + ctx->data->vio_dbg0_offset;
> >>> +	vio_dbg1_reg = ctx->infra_base + ctx->data->vio_dbg1_offset;
> >>> +
> >>> +	vio_dbgs.vio_dbg0 = readl(vio_dbg0_reg);
> >>> +	vio_dbgs.vio_dbg1 = readl(vio_dbg1_reg);
> >>> +
> >>> +	/* Print violation information */
> >>> +	if (vio_dbgs.dbg0_bits.vio_w)
> >>> +		dev_info(ctx->dev, "Write Violation\n");
> >>> +	else if (vio_dbgs.dbg0_bits.vio_r)
> >>> +		dev_info(ctx->dev, "Read Violation\n");
> >>> +
> >>> +	dev_info(ctx->dev, "Bus ID:0x%x, Dom ID:0x%x, Vio Addr:0x%x\n",
> >>> +		 vio_dbgs.dbg0_bits.mstid, vio_dbgs.dbg0_bits.dmnid,
> >>> +		 vio_dbgs.vio_dbg1);
> >>> +}
> >>> +
> >>> +/*
> >>> + * 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)
> >>
> >> static irqreturn_t devapc_violation_irq(int irq_number, void *data)
> >> {
> >> 	struct mtk_devapc_context *ctx = data;
> > 
> > Okay, I'll fix it on next patch.
> > Thanks
> > 
> >>
> >>> +{
> >>> +	while (devapc_sync_vio_dbg(ctx))
> >>> +		devapc_extract_vio_dbg(ctx);
> >>> +
> >>> +	clear_vio_status(ctx);
> >>> +
> >>> +	return IRQ_HANDLED;
> >>> +}
> >>> +
> >>> +/*
> >>> + * start_devapc - unmask slave's irq to start receiving devapc violation.
> >>> + */
> >>> +static void start_devapc(struct mtk_devapc_context *ctx)
> >>> +{
> >>> +	writel(BIT(31), ctx->infra_base + ctx->data->apc_con_offset);
> >>> +
> >>> +	mask_module_irq(ctx, false);
> >>> +}
> >>> +
> >>> +/*
> >>> + * stop_devapc - mask slave's irq to stop service.
> >>> + */
> >>> +static void stop_devapc(struct mtk_devapc_context *ctx)
> >>> +{
> >>> +	mask_module_irq(ctx, true);
> >>> +
> >>> +	writel(BIT(2), ctx->infra_base + ctx->data->apc_con_offset);
> >>> +}
> >>> +
> >>> +static const struct mtk_devapc_data devapc_mt6779 = {
> >>> +	.vio_idx_num = 511,
> >>> +	.vio_mask_offset = 0x0,
> >>> +	.vio_sta_offset = 0x400,
> >>> +	.vio_dbg0_offset = 0x900,
> >>> +	.vio_dbg1_offset = 0x904,
> >>> +	.apc_con_offset = 0xF00,
> >>> +	.vio_shift_sta_offset = 0xF10,
> >>> +	.vio_shift_sel_offset = 0xF14,
> >>> +	.vio_shift_con_offset = 0xF20,
> >>> +};
> >>> +
> >>> +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;
> >>> +	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->data = of_device_get_match_data(&pdev->dev);
> >>> +	ctx->dev = &pdev->dev;
> >>> +
> >>> +	ctx->infra_base = of_iomap(node, 0);
> >>
> >> Does this mean the device is part of the infracfg block?
> >> I wasn't able to find any information about it.
> > 
> > I'm not sure why you would ask infracfg block. devapc is parts of our
> > SoC infra, it's different with infracfg.
> > 
> 
> I'm asking because I want to understand the HW better. I'm not able to find any 
> information in the datasheets. I want to avoid a situation as we had with the 
> MMSYS where a clock driver was submitted first and later on we realized that 
> MMSYS is much more then that and we had to work hard to get the driver right.
> 
> Now it's happening with SCPSYS, where a driver with the scpsys compatible was 
> send years ago. But SCPSYS is much more then the driver submitted. In this case 
> we opted to write a new driver, but moving from one driver to another one is 
> painfull and full of problems. For that I want to make sure we fully understand 
> Device APC (by the way, what does APC stands for?). Is it a totally independent 
> HW block or is it part of a subsystem, like for example SCP?
> 
> Regards,
> Matthias

It's a totally independent HW block instead of a subsystem.
I think it's more simple than MMSYS or SCPSYS. But if you would like to
understand more about this HW, we could find another way/channel to
introduce it.


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-10-08  9:39         ` Neal Liu
@ 2020-10-09 12:34           ` Matthias Brugger
  2020-10-12  3:24             ` Neal Liu
  0 siblings, 1 reply; 19+ messages in thread
From: Matthias Brugger @ 2020-10-09 12:34 UTC (permalink / raw)
  To: Neal Liu
  Cc: Chun-Kuang Hu, wsd_upstream, devicetree, lkml, Rob Herring,
	linux-mediatek, linux-arm-kernel



On 08/10/2020 11:39, Neal Liu wrote:
> On Thu, 2020-10-08 at 10:45 +0200, Matthias Brugger wrote:
>>
>> On 08/10/2020 04:35, Neal Liu wrote:
>>> On Wed, 2020-10-07 at 12:44 +0200, Matthias Brugger wrote:
>>>>
>>>> On 27/08/2020 05:06, Neal Liu wrote:
[...]

>>>>> +static int mtk_devapc_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +	struct device_node *node = pdev->dev.of_node;
>>>>> +	struct mtk_devapc_context *ctx;
>>>>> +	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->data = of_device_get_match_data(&pdev->dev);
>>>>> +	ctx->dev = &pdev->dev;
>>>>> +
>>>>> +	ctx->infra_base = of_iomap(node, 0);
>>>>
>>>> Does this mean the device is part of the infracfg block?
>>>> I wasn't able to find any information about it.
>>>
>>> I'm not sure why you would ask infracfg block. devapc is parts of our
>>> SoC infra, it's different with infracfg.
>>>
>>
>> I'm asking because I want to understand the HW better. I'm not able to find any
>> information in the datasheets. I want to avoid a situation as we had with the
>> MMSYS where a clock driver was submitted first and later on we realized that
>> MMSYS is much more then that and we had to work hard to get the driver right.
>>
>> Now it's happening with SCPSYS, where a driver with the scpsys compatible was
>> send years ago. But SCPSYS is much more then the driver submitted. In this case
>> we opted to write a new driver, but moving from one driver to another one is
>> painfull and full of problems. For that I want to make sure we fully understand
>> Device APC (by the way, what does APC stands for?). Is it a totally independent
>> HW block or is it part of a subsystem, like for example SCP?
>>
>> Regards,
>> Matthias
> 
> It's a totally independent HW block instead of a subsystem.
> I think it's more simple than MMSYS or SCPSYS. But if you would like to
> understand more about this HW, we could find another way/channel to
> introduce it.
> 

If it's a independent HW block, then we are good. No further information needed 
by me. I'd just advise to rename the infra_base to something like base, as it 
made me confuse.

Thanks!
Matthias

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-10-09 12:34           ` Matthias Brugger
@ 2020-10-12  3:24             ` Neal Liu
  0 siblings, 0 replies; 19+ messages in thread
From: Neal Liu @ 2020-10-12  3:24 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: Chun-Kuang Hu, wsd_upstream, devicetree, lkml, Rob Herring,
	linux-mediatek, Neal Liu, linux-arm-kernel

On Fri, 2020-10-09 at 14:34 +0200, Matthias Brugger wrote:
> 
> On 08/10/2020 11:39, Neal Liu wrote:
> > On Thu, 2020-10-08 at 10:45 +0200, Matthias Brugger wrote:
> >>
> >> On 08/10/2020 04:35, Neal Liu wrote:
> >>> On Wed, 2020-10-07 at 12:44 +0200, Matthias Brugger wrote:
> >>>>
> >>>> On 27/08/2020 05:06, Neal Liu wrote:
> [...]
> 
> >>>>> +static int mtk_devapc_probe(struct platform_device *pdev)
> >>>>> +{
> >>>>> +	struct device_node *node = pdev->dev.of_node;
> >>>>> +	struct mtk_devapc_context *ctx;
> >>>>> +	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->data = of_device_get_match_data(&pdev->dev);
> >>>>> +	ctx->dev = &pdev->dev;
> >>>>> +
> >>>>> +	ctx->infra_base = of_iomap(node, 0);
> >>>>
> >>>> Does this mean the device is part of the infracfg block?
> >>>> I wasn't able to find any information about it.
> >>>
> >>> I'm not sure why you would ask infracfg block. devapc is parts of our
> >>> SoC infra, it's different with infracfg.
> >>>
> >>
> >> I'm asking because I want to understand the HW better. I'm not able to find any
> >> information in the datasheets. I want to avoid a situation as we had with the
> >> MMSYS where a clock driver was submitted first and later on we realized that
> >> MMSYS is much more then that and we had to work hard to get the driver right.
> >>
> >> Now it's happening with SCPSYS, where a driver with the scpsys compatible was
> >> send years ago. But SCPSYS is much more then the driver submitted. In this case
> >> we opted to write a new driver, but moving from one driver to another one is
> >> painfull and full of problems. For that I want to make sure we fully understand
> >> Device APC (by the way, what does APC stands for?). Is it a totally independent
> >> HW block or is it part of a subsystem, like for example SCP?
> >>
> >> Regards,
> >> Matthias
> > 
> > It's a totally independent HW block instead of a subsystem.
> > I think it's more simple than MMSYS or SCPSYS. But if you would like to
> > understand more about this HW, we could find another way/channel to
> > introduce it.
> > 
> 
> If it's a independent HW block, then we are good. No further information needed 
> by me. I'd just advise to rename the infra_base to something like base, as it 
> made me confuse.
> 
> Thanks!
> Matthias

You can imagine that infra_base means infra devapc base address for
MT6779. In 5G platforms, MediaTek infrastructure would separate into
multiple parts, so does devapc HW. And devapc would be like:
infra_base, peri_base, peri2_base, ...

Thanks.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-10-08  2:35     ` Neal Liu
  2020-10-08  8:45       ` Matthias Brugger
@ 2020-10-15  2:13       ` Neal Liu
  2020-10-15  9:13         ` Matthias Brugger
  1 sibling, 1 reply; 19+ messages in thread
From: Neal Liu @ 2020-10-15  2:13 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: Chun-Kuang Hu, wsd_upstream, devicetree, lkml, Rob Herring,
	linux-mediatek, Neal Liu, linux-arm-kernel

On Thu, 2020-10-08 at 10:35 +0800, Neal Liu wrote:
> On Wed, 2020-10-07 at 12:44 +0200, Matthias Brugger wrote:
> > 
> > On 27/08/2020 05:06, Neal Liu wrote:
[...]

> > > +static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
> > > +{
> > > +	void __iomem *pd_vio_shift_sta_reg;
> > > +	void __iomem *pd_vio_shift_sel_reg;
> > > +	void __iomem *pd_vio_shift_con_reg;
> > > +	int min_shift_group;
> > > +	int ret;
> > > +	u32 val;
> > > +
> > > +	pd_vio_shift_sta_reg = ctx->infra_base +
> > > +			       ctx->data->vio_shift_sta_offset;
> > > +	pd_vio_shift_sel_reg = ctx->infra_base +
> > > +			       ctx->data->vio_shift_sel_offset;
> > > +	pd_vio_shift_con_reg = ctx->infra_base +
> > > +			       ctx->data->vio_shift_con_offset;
> > > +
> > > +	/* Find the minimum shift group which has violation */
> > > +	val = readl(pd_vio_shift_sta_reg);
> > > +	if (!val)
> > > +		return false;
> > 
> > So bit 0 of selection register (pd_vio_shift_sel_reg) does not represent a 
> > violation group?
> > I don't know how the HW works, but is seems odd to me. In case that's bit 0 
> > actually doesn't represent anything: how can an interrupt be triggered without 
> > any debug information present (means val == 0)?
> 
> This check implies HW status has something wrong. It cannot get any
> debug information for this case.
> It won't happen in normal scenario. Should we remove this check?
> 

Sorry, I missed the most common part. Is function is in the while loop:
while (devapc_sync_vio_dbg(ctx))
...

We keep find the minimum bit in pd_vio_shift_sta_reg to get the
violation information, (pd_vio_shift_sta_reg might raise multiple bits)
until all raised bit (shift group) has been handled.
So I don't think it's necessary to add WARN message in this case.
Thanks

> > 
> > > +
> > > +	min_shift_group = __ffs(val);
> > > +
> > > +	/* Assign the group to sync */
> > > +	writel(0x1 << min_shift_group, pd_vio_shift_sel_reg);
> > > +
> > > +	/* Start syncing */
> > > +	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__);
> > 
> > In which case this can happen? I'm asking, because we are calling 
> > devapc_sync_vio_dbg() in a while loop that could make the kernel hang here.
> > 
> > Do I understand correctly, that we are using the while loop, because there can 
> > be more then one violation group which got triggered (read, more then one bit is 
> > set in pd_vio_shift_sta_reg)? Would it make more sense then to read the register 
> > once and do all the shift operation for all groups which bit set to 1 in the 
> > shift status register?
> 
> Yes, your understanding is correct.
> This check also implies HW status has something wrong. We return false
> to skip further violation info dump.
> How could this case make the kernel hang?
> 
> > 
> > > +		return false;
> > > +	}
> > > +
> > > +	/* Stop syncing */
> > > +	writel(0x0, pd_vio_shift_con_reg);
> > > +
> > > +	/* Write clear */
> > > +	writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +/*
> > > + * devapc_extract_vio_dbg - extract full violation information after doing
> > > + *                          shift mechanism.
> > > + */
> > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > +{
> > > +	struct mtk_devapc_vio_dbgs vio_dbgs;
> > > +	void __iomem *vio_dbg0_reg;
> > > +	void __iomem *vio_dbg1_reg;
> > > +
> > > +	vio_dbg0_reg = ctx->infra_base + ctx->data->vio_dbg0_offset;
> > > +	vio_dbg1_reg = ctx->infra_base + ctx->data->vio_dbg1_offset;
> > > +
> > > +	vio_dbgs.vio_dbg0 = readl(vio_dbg0_reg);
> > > +	vio_dbgs.vio_dbg1 = readl(vio_dbg1_reg);
> > > +
> > > +	/* Print violation information */
> > > +	if (vio_dbgs.dbg0_bits.vio_w)
> > > +		dev_info(ctx->dev, "Write Violation\n");
> > > +	else if (vio_dbgs.dbg0_bits.vio_r)
> > > +		dev_info(ctx->dev, "Read Violation\n");
> > > +
> > > +	dev_info(ctx->dev, "Bus ID:0x%x, Dom ID:0x%x, Vio Addr:0x%x\n",
> > > +		 vio_dbgs.dbg0_bits.mstid, vio_dbgs.dbg0_bits.dmnid,
> > > +		 vio_dbgs.vio_dbg1);
> > > +}
> > > +
> > > +/*
> > > + * 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)
> > 
> > static irqreturn_t devapc_violation_irq(int irq_number, void *data)
> > {
> > 	struct mtk_devapc_context *ctx = data;
> 
> Okay, I'll fix it on next patch.
> Thanks
> 
> > 
> > > +{
> > > +	while (devapc_sync_vio_dbg(ctx))
> > > +		devapc_extract_vio_dbg(ctx);
> > > +
> > > +	clear_vio_status(ctx);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +/*
> > > + * start_devapc - unmask slave's irq to start receiving devapc violation.
> > > + */
> > > +static void start_devapc(struct mtk_devapc_context *ctx)
> > > +{
> > > +	writel(BIT(31), ctx->infra_base + ctx->data->apc_con_offset);
> > > +
> > > +	mask_module_irq(ctx, false);
> > > +}
> > > +
> > > +/*
> > > + * stop_devapc - mask slave's irq to stop service.
> > > + */
> > > +static void stop_devapc(struct mtk_devapc_context *ctx)
> > > +{
> > > +	mask_module_irq(ctx, true);
> > > +
> > > +	writel(BIT(2), ctx->infra_base + ctx->data->apc_con_offset);
> > > +}
> > > +
> > > +static const struct mtk_devapc_data devapc_mt6779 = {
> > > +	.vio_idx_num = 511,
> > > +	.vio_mask_offset = 0x0,
> > > +	.vio_sta_offset = 0x400,
> > > +	.vio_dbg0_offset = 0x900,
> > > +	.vio_dbg1_offset = 0x904,
> > > +	.apc_con_offset = 0xF00,
> > > +	.vio_shift_sta_offset = 0xF10,
> > > +	.vio_shift_sel_offset = 0xF14,
> > > +	.vio_shift_con_offset = 0xF20,
> > > +};
> > > +
> > > +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;
> > > +	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->data = of_device_get_match_data(&pdev->dev);
> > > +	ctx->dev = &pdev->dev;
> > > +
> > > +	ctx->infra_base = of_iomap(node, 0);
> > 
> > Does this mean the device is part of the infracfg block?
> > I wasn't able to find any information about it.
> 
> I'm not sure why you would ask infracfg block. devapc is parts of our
> SoC infra, it's different with infracfg.
> 
> > 
> > > +	if (!ctx->infra_base)
> > > +		return -EINVAL;
> > > +
> > > +	devapc_irq = irq_of_parse_and_map(node, 0);
> > > +	if (!devapc_irq)
> > > +		return -EINVAL;
> > > +
> > > +	ctx->infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> > > +	if (IS_ERR(ctx->infra_clk))
> > > +		return -EINVAL;
> > > +
> > > +	if (clk_prepare_enable(ctx->infra_clk))
> > > +		return -EINVAL;
> > > +
> > > +	ret = devm_request_irq(&pdev->dev, devapc_irq,
> > > +			       (irq_handler_t)devapc_violation_irq,
> > 
> > No cast should be needed.
> 
> Okay, I'll remove it on next patch.
> Thanks
> 
> > 
> > > +			       IRQF_TRIGGER_NONE, "devapc", ctx);
> > > +	if (ret) {
> > > +		clk_disable_unprepare(ctx->infra_clk);
> > > +		return ret;
> > > +	}
> > > +
> > > +	platform_set_drvdata(pdev, ctx);
> > > +
> > > +	start_devapc(ctx);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int mtk_devapc_remove(struct platform_device *pdev)
> > > +{
> > > +	struct mtk_devapc_context *ctx = platform_get_drvdata(pdev);
> > > +
> > > +	stop_devapc(ctx);
> > > +
> > > +	clk_disable_unprepare(ctx->infra_clk);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver mtk_devapc_driver = {
> > > +	.probe = mtk_devapc_probe,
> > > +	.remove = mtk_devapc_remove,
> > > +	.driver = {
> > > +		.name = KBUILD_MODNAME,
> > 
> > .name = "mtk-devapc",
> 
> Okay, I'll add it on next patch.
> Thanks
> 
> > 
> > Regards,
> > Matthias
> 
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
  2020-10-15  2:13       ` Neal Liu
@ 2020-10-15  9:13         ` Matthias Brugger
  0 siblings, 0 replies; 19+ messages in thread
From: Matthias Brugger @ 2020-10-15  9:13 UTC (permalink / raw)
  To: Neal Liu
  Cc: Chun-Kuang Hu, wsd_upstream, devicetree, lkml, Rob Herring,
	linux-mediatek, linux-arm-kernel



On 15/10/2020 04:13, Neal Liu wrote:
> On Thu, 2020-10-08 at 10:35 +0800, Neal Liu wrote:
>> On Wed, 2020-10-07 at 12:44 +0200, Matthias Brugger wrote:
>>>
>>> On 27/08/2020 05:06, Neal Liu wrote:
> [...]
> 
>>>> +static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
>>>> +{
>>>> +	void __iomem *pd_vio_shift_sta_reg;
>>>> +	void __iomem *pd_vio_shift_sel_reg;
>>>> +	void __iomem *pd_vio_shift_con_reg;
>>>> +	int min_shift_group;
>>>> +	int ret;
>>>> +	u32 val;
>>>> +
>>>> +	pd_vio_shift_sta_reg = ctx->infra_base +
>>>> +			       ctx->data->vio_shift_sta_offset;
>>>> +	pd_vio_shift_sel_reg = ctx->infra_base +
>>>> +			       ctx->data->vio_shift_sel_offset;
>>>> +	pd_vio_shift_con_reg = ctx->infra_base +
>>>> +			       ctx->data->vio_shift_con_offset;
>>>> +
>>>> +	/* Find the minimum shift group which has violation */
>>>> +	val = readl(pd_vio_shift_sta_reg);
>>>> +	if (!val)
>>>> +		return false;
>>>
>>> So bit 0 of selection register (pd_vio_shift_sel_reg) does not represent a
>>> violation group?
>>> I don't know how the HW works, but is seems odd to me. In case that's bit 0
>>> actually doesn't represent anything: how can an interrupt be triggered without
>>> any debug information present (means val == 0)?
>>
>> This check implies HW status has something wrong. It cannot get any
>> debug information for this case.
>> It won't happen in normal scenario. Should we remove this check?
>>
> 
> Sorry, I missed the most common part. Is function is in the while loop:
> while (devapc_sync_vio_dbg(ctx))
> ...
> 
> We keep find the minimum bit in pd_vio_shift_sta_reg to get the
> violation information, (pd_vio_shift_sta_reg might raise multiple bits)
> until all raised bit (shift group) has been handled.
> So I don't think it's necessary to add WARN message in this case.
> Thanks
> 

Correct we would get a warning every time once we have read and cleared all 
violations, but we are only concerned about the IRQ triggered with no violation 
present. But we can skip that check for now, if we see that there could be a HW 
error we can think about it in the future.

Regards,
Matthias

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, back to index

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-27  3:06 [PATCH v7] Add MediaTek MT6779 devapc driver Neal Liu
2020-08-27  3:06 ` [PATCH v7 1/2] dt-bindings: devapc: add bindings for mtk-devapc Neal Liu
2020-08-27  3:06 ` [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver Neal Liu
2020-10-02 16:24   ` Chun-Kuang Hu
2020-10-05  7:20     ` Neal Liu
2020-10-07 10:44   ` Matthias Brugger
2020-10-08  2:35     ` Neal Liu
2020-10-08  8:45       ` Matthias Brugger
2020-10-08  9:39         ` Neal Liu
2020-10-09 12:34           ` Matthias Brugger
2020-10-12  3:24             ` Neal Liu
2020-10-15  2:13       ` Neal Liu
2020-10-15  9:13         ` Matthias Brugger
2020-09-02  6:40 ` [PATCH v7] Add MediaTek MT6779 " Neal Liu
2020-09-09  8:37   ` Neal Liu
2020-09-16  8:58     ` Neal Liu
2020-09-22  7:13       ` Neal Liu
2020-09-30  7:10         ` Neal Liu
2020-10-02 16:17           ` Chun-Kuang Hu

Linux-mediatek Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mediatek/0 linux-mediatek/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mediatek linux-mediatek/ https://lore.kernel.org/linux-mediatek \
		linux-mediatek@lists.infradead.org
	public-inbox-index linux-mediatek

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mediatek


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git