* Add MediaTek MT6873 devapc driver
@ 2020-06-09 10:24 Neal Liu
2020-06-09 10:24 ` [PATCH 1/2] dt-bindings: devapc: add bindings for devapc-mt6873 Neal Liu
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Neal Liu @ 2020-06-09 10:24 UTC (permalink / raw)
To: Rob Herring, Matthias Brugger
Cc: devicetree, wsd_upstream, linux-kernel, linux-mediatek, Neal Liu,
linux-arm-kernel
These patch series introduce a MediaTek MT6873 devapc driver.
MT6873 bus frabric provides TrustZone security support and data
protection to prevent slaves from being accessed by unexpected
masters.
The security violations are 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 devapc-mt6873 driver. The violation
information is printed in order to find the murderer.
*** BLURB HERE ***
Neal Liu (2):
dt-bindings: devapc: add bindings for devapc-mt6873
soc: mediatek: devapc: add devapc-mt6873 driver
.../soc/mediatek/devapc/devapc-mt6873.yaml | 61 +
drivers/soc/mediatek/Kconfig | 6 +
drivers/soc/mediatek/Makefile | 1 +
drivers/soc/mediatek/devapc/Kconfig | 25 +
drivers/soc/mediatek/devapc/Makefile | 13 +
drivers/soc/mediatek/devapc/devapc-mt6873.c | 1733 +++++++++++++++++
drivers/soc/mediatek/devapc/devapc-mt6873.h | 130 ++
.../soc/mediatek/devapc/devapc-mtk-multi-ao.c | 1019 ++++++++++
.../soc/mediatek/devapc/devapc-mtk-multi-ao.h | 183 ++
include/linux/soc/mediatek/devapc_public.h | 41 +
10 files changed, 3212 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml
create mode 100644 drivers/soc/mediatek/devapc/Kconfig
create mode 100644 drivers/soc/mediatek/devapc/Makefile
create mode 100644 drivers/soc/mediatek/devapc/devapc-mt6873.c
create mode 100644 drivers/soc/mediatek/devapc/devapc-mt6873.h
create mode 100644 drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.c
create mode 100644 drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.h
create mode 100644 include/linux/soc/mediatek/devapc_public.h
--
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/2] dt-bindings: devapc: add bindings for devapc-mt6873
2020-06-09 10:24 Add MediaTek MT6873 devapc driver Neal Liu
@ 2020-06-09 10:24 ` Neal Liu
2020-06-09 17:27 ` Rob Herring
[not found] ` <1591698261-22639-3-git-send-email-neal.liu@mediatek.com>
2020-06-09 17:32 ` Add MediaTek MT6873 devapc driver Rob Herring
2 siblings, 1 reply; 20+ messages in thread
From: Neal Liu @ 2020-06-09 10:24 UTC (permalink / raw)
To: Rob Herring, Matthias Brugger
Cc: devicetree, wsd_upstream, linux-kernel, linux-mediatek, Neal Liu,
linux-arm-kernel
Add bindings for MT6873 devapc.
Signed-off-by: Neal Liu <neal.liu@mediatek.com>
---
.../soc/mediatek/devapc/devapc-mt6873.yaml | 61 ++++++++++++++++++++
1 file changed, 61 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml
diff --git a/Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml b/Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml
new file mode 100644
index 0000000..73a14b8d
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml
@@ -0,0 +1,61 @@
+# 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/devapc-mt6873.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: MediaTek MT6873 Device Access Permission Control driver
+
+description: |
+ MediaTek MT6873 bus frabric provides TrustZone security support and data
+ protection to prevent slaves from being accessed by unexpected masters.
+ The security violations are logged and sent to the processor for further
+ analysis and countermeasures.
+
+maintainer:
+ - Neal Liu <neal.liu@mediatek.com>
+
+properties:
+ compatible:
+ enum:
+ - mediatek,mt6873-devapc
+
+ reg:
+ description: The base address of devapc register bank
+ maxItems: 5
+
+ 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
+
+additionalProperties: false
+
+examples:
+ - |
+ devapc: devapc@10207000 {
+ compatible = "mediatek,mt6873-devapc";
+ reg = <0 0x10207000 0 0x1000>,
+ <0 0x10274000 0 0x1000>,
+ <0 0x10275000 0 0x1000>,
+ <0 0x11020000 0 0x1000>,
+ <0 0x1020e000 0 0x1000>;
+ interrupts = <GIC_SPI 187 IRQ_TYPE_LEVEL_HIGH 0>;
+ clocks = <&infracfg CLK_INFRA_DEVICE_APC>;
+ clock-names = "devapc-infra-clock";
+ };
--
1.7.9.5
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
[not found] ` <1591698261-22639-3-git-send-email-neal.liu@mediatek.com>
@ 2020-06-09 16:01 ` Chun-Kuang Hu
2020-06-11 9:26 ` Neal Liu
2020-06-12 23:20 ` Chun-Kuang Hu
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Chun-Kuang Hu @ 2020-06-09 16:01 UTC (permalink / raw)
To: Neal Liu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Linux ARM
Hi, Neal:
Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
>
> MT6873 bus frabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violations are 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 devapc-mt6873 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 | 6 +
> drivers/soc/mediatek/Makefile | 1 +
> drivers/soc/mediatek/devapc/Kconfig | 25 +
> drivers/soc/mediatek/devapc/Makefile | 13 +
> drivers/soc/mediatek/devapc/devapc-mt6873.c | 1733 +++++++++++++++++++++
> drivers/soc/mediatek/devapc/devapc-mt6873.h | 130 ++
> drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.c | 1019 ++++++++++++
> drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.h | 183 +++
> include/linux/soc/mediatek/devapc_public.h | 41 +
> 9 files changed, 3151 insertions(+)
> create mode 100644 drivers/soc/mediatek/devapc/Kconfig
> create mode 100644 drivers/soc/mediatek/devapc/Makefile
> create mode 100644 drivers/soc/mediatek/devapc/devapc-mt6873.c
> create mode 100644 drivers/soc/mediatek/devapc/devapc-mt6873.h
> create mode 100644 drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.c
> create mode 100644 drivers/soc/mediatek/devapc/devapc-mtk-multi-ao.h
> create mode 100644 include/linux/soc/mediatek/devapc_public.h
>
> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> index 2114b56..cc46f50 100644
> --- a/drivers/soc/mediatek/Kconfig
> +++ b/drivers/soc/mediatek/Kconfig
> @@ -44,4 +44,10 @@ config MTK_SCPSYS
> Say yes here to add support for the MediaTek SCPSYS power domain
> driver.
>
> +menu "Security"
> +
> +source "drivers/soc/mediatek/devapc/Kconfig"
> +
> +endmenu # Security
> +
> endmenu
> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> index b017330..7154a2a 100644
> --- a/drivers/soc/mediatek/Makefile
> +++ b/drivers/soc/mediatek/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> +obj-$(CONFIG_MTK_DEVAPC) += devapc/
alphabetic order.
> diff --git a/drivers/soc/mediatek/devapc/Kconfig b/drivers/soc/mediatek/devapc/Kconfig
> new file mode 100644
> index 0000000..9428360
> --- /dev/null
> +++ b/drivers/soc/mediatek/devapc/Kconfig
> @@ -0,0 +1,25 @@
> +config MTK_DEVAPC
> + tristate "Mediatek Device APC Support"
> + help
> + Device APC is a kernel driver controlling internal device security.
> + If someone tries to access a device, which is not allowed by the
> + device, it cannot access the device and will get a violation
> + interrupt. Device APC prevents malicious access to internal devices.
> +
> +config DEVAPC_ARCH_MULTI
> + tristate "Mediatek Device APC driver architecture multi"
> + help
> + Say yes here to enable support Mediatek
> + Device APC driver which is based on Infra
> + architecture.
> + This architecture supports multiple Infra AO.
> +
> +config DEVAPC_MT6873
> + tristate "Mediatek MT6873 Device APC driver"
> + select MTK_DEVAPC
> + select DEVAPC_ARCH_MULTI
> + help
> + Say yes here to enable support Mediatek MT6873
> + Device APC driver.
> + This driver is combined with DEVAPC_ARCH_MULTI for
> + common handle flow.
[snip]
> +static struct mtk_devapc_context {
> + struct clk *devapc_infra_clk;
> + u32 devapc_irq;
> +
> + /* HW reg mapped addr */
> + void __iomem *devapc_pd_base[4];
> + void __iomem *infracfg_base;
> +
> + struct mtk_devapc_soc *soc;
> +} mtk_devapc_ctx[1];
> +
> +static LIST_HEAD(viocb_list);
> +static DEFINE_SPINLOCK(devapc_lock);
Move global variable into struct mtk_devapc_context .
> +
> +/*
> + * mtk_devapc_pd_get - get devapc pd_types of register address.
> + *
> + * Returns the value of reg addr
> + */
> +static void __iomem *mtk_devapc_pd_get(int slave_type,
> + enum DEVAPC_PD_REG_TYPE pd_reg_type,
> + u32 index)
> +{
> + struct mtk_devapc_vio_info *vio_info = mtk_devapc_ctx->soc->vio_info;
> + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> + const u32 *devapc_pds = mtk_devapc_ctx->soc->devapc_pds;
> + void __iomem *reg;
> +
> + if (!devapc_pds)
> + return NULL;
> +
> + if ((slave_type < slave_type_num &&
> + index < vio_info->vio_mask_sta_num[slave_type]) &&
> + pd_reg_type < PD_REG_TYPE_NUM) {
> + reg = mtk_devapc_ctx->devapc_pd_base[slave_type] +
> + devapc_pds[pd_reg_type];
> +
> + if (pd_reg_type == VIO_MASK || pd_reg_type == VIO_STA)
> + reg += 0x4 * index;
> +
> + } else {
> + pr_err(PFX "%s:0x%x or %s:0x%x or %s:0x%x is out of boundary\n",
> + "slave_type", slave_type,
Move "slave_type" into format string.
> + "pd_reg_type", pd_reg_type,
> + "index", index);
> + return NULL;
> + }
> +
> + return reg;
> +}
> +
[snip]
> +
> +/*
> + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> + * violation information including which master violates
> + * access slave.
> + */
> +static irqreturn_t devapc_violation_irq(int irq_number, void *dev_id)
> +{
> + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> + const struct mtk_device_info **device_info;
> + struct mtk_devapc_vio_info *vio_info;
> + int slave_type, vio_idx, index;
> + const char *vio_master;
> + unsigned long flags;
> + bool normal;
> + u8 perm;
> +
> + spin_lock_irqsave(&devapc_lock, flags);
> +
> + device_info = mtk_devapc_ctx->soc->device_info;
> + vio_info = mtk_devapc_ctx->soc->vio_info;
> + normal = false;
> + vio_idx = -1;
> + index = -1;
> +
> + /* There are multiple DEVAPC_PD */
> + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> + if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> + if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> + &index))
> + continue;
> +
> + /* Ensure that violation info are written before
> + * further operations
> + */
> + smp_mb();
> + normal = true;
> +
> + mask_module_irq(slave_type, vio_idx, true);
> +
> + if (clear_vio_status(slave_type, vio_idx))
> + pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> + "clear vio status failed",
> + "slave_type", slave_type,
> + "vio_index", vio_idx);
> +
> + perm = get_permission(slave_type, index, vio_info->domain_id);
> +
> + vio_master = mtk_devapc_ctx->soc->master_get
> + (vio_info->master_id,
> + vio_info->vio_addr,
> + slave_type,
> + vio_info->shift_sta_bit,
> + vio_info->domain_id);
Call mt6873_bus_id_to_master() directly. For first patch, make things
as simple as possible.
> +
> + if (!vio_master) {
> + pr_warn(PFX "master_get failed\n");
> + vio_master = "UNKNOWN_MASTER";
> + }
> +
> + pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> + "Violation", "slave_type", slave_type,
> + "sys_index",
> + device_info[slave_type][index].sys_index,
> + "ctrl_index",
> + device_info[slave_type][index].ctrl_index,
> + "vio_index",
> + device_info[slave_type][index].vio_index);
> +
> + pr_info(PFX "%s %s %s %s\n",
> + "Violation - master:", vio_master,
> + "access violation slave:",
> + device_info[slave_type][index].device);
> +
> + devapc_vio_reason(perm);
> +
> + devapc_extra_handler(slave_type, vio_master, vio_idx,
> + vio_info->vio_addr);
> +
> + mask_module_irq(slave_type, vio_idx, false);
> + }
> +
> + if (normal) {
> + spin_unlock_irqrestore(&devapc_lock, flags);
> + return IRQ_HANDLED;
> + }
> +
> + spin_unlock_irqrestore(&devapc_lock, flags);
> + return IRQ_HANDLED;
> +}
> +
[snip]
> +uint32_t devapc_vio_check(void);
> +void dump_dbg_info(void);
> +void register_devapc_vio_callback(struct devapc_vio_callbacks *viocb);
> +void devapc_catch_illegal_range(phys_addr_t phys_addr, size_t size);
devapc_catch_illegal_range() is useless, so remove it.
Regards,
Chun-Kuang.
> +
> +#endif /* __DEVAPC_PUBLIC_H__ */
> +
> --
> 1.7.9.5
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: devapc: add bindings for devapc-mt6873
2020-06-09 10:24 ` [PATCH 1/2] dt-bindings: devapc: add bindings for devapc-mt6873 Neal Liu
@ 2020-06-09 17:27 ` Rob Herring
0 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2020-06-09 17:27 UTC (permalink / raw)
To: Neal Liu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring,
linux-mediatek, Matthias Brugger, linux-arm-kernel
On Tue, 09 Jun 2020 18:24:20 +0800, Neal Liu wrote:
> Add bindings for MT6873 devapc.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> ---
> .../soc/mediatek/devapc/devapc-mt6873.yaml | 61 ++++++++++++++++++++
> 1 file changed, 61 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml: Additional properties are not allowed ('maintainer' was unexpected)
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml: 'maintainers' is a required property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml: Additional properties are not allowed ('maintainer' was unexpected)
Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml: ignoring, error in schema:
warning: no schema found in file: ./Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml: ignoring, error in schema:
warning: no schema found in file: ./Documentation/devicetree/bindings/soc/mediatek/devapc/devapc-mt6873.yaml
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1305778
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade
Please check and re-submit.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Add MediaTek MT6873 devapc driver
2020-06-09 10:24 Add MediaTek MT6873 devapc driver Neal Liu
2020-06-09 10:24 ` [PATCH 1/2] dt-bindings: devapc: add bindings for devapc-mt6873 Neal Liu
[not found] ` <1591698261-22639-3-git-send-email-neal.liu@mediatek.com>
@ 2020-06-09 17:32 ` Rob Herring
2020-06-24 3:51 ` Neal Liu
2 siblings, 1 reply; 20+ messages in thread
From: Rob Herring @ 2020-06-09 17:32 UTC (permalink / raw)
To: Neal Liu
Cc: devicetree, wsd_upstream, linux-kernel, linux-mediatek,
Matthias Brugger, linux-arm-kernel
On Tue, Jun 09, 2020 at 06:24:19PM +0800, Neal Liu wrote:
> These patch series introduce a MediaTek MT6873 devapc driver.
>
> MT6873 bus frabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violations are 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 devapc-mt6873 driver. The violation
> information is printed in order to find the murderer.
There's also a proposed driver with similar functionality[1]. Come up
with a common solution.
Rob
[1] https://lore.kernel.org/linux-arm-kernel/20200128153806.7780-1-benjamin.gaignard@st.com/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-09 16:01 ` [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver Chun-Kuang Hu
@ 2020-06-11 9:26 ` Neal Liu
2020-06-11 11:01 ` Chun-Kuang Hu
0 siblings, 1 reply; 20+ messages in thread
From: Neal Liu @ 2020-06-11 9:26 UTC (permalink / raw)
To: Chun-Kuang Hu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Linux ARM
On Wed, 2020-06-10 at 00:01 +0800, Chun-Kuang Hu wrote:
Hi Chun-Kuang,
[snip]
> > +
> > +/*
> > + * mtk_devapc_pd_get - get devapc pd_types of register address.
> > + *
> > + * Returns the value of reg addr
> > + */
> > +static void __iomem *mtk_devapc_pd_get(int slave_type,
> > + enum DEVAPC_PD_REG_TYPE pd_reg_type,
> > + u32 index)
> > +{
> > + struct mtk_devapc_vio_info *vio_info = mtk_devapc_ctx->soc->vio_info;
> > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > + const u32 *devapc_pds = mtk_devapc_ctx->soc->devapc_pds;
> > + void __iomem *reg;
> > +
> > + if (!devapc_pds)
> > + return NULL;
> > +
> > + if ((slave_type < slave_type_num &&
> > + index < vio_info->vio_mask_sta_num[slave_type]) &&
> > + pd_reg_type < PD_REG_TYPE_NUM) {
> > + reg = mtk_devapc_ctx->devapc_pd_base[slave_type] +
> > + devapc_pds[pd_reg_type];
> > +
> > + if (pd_reg_type == VIO_MASK || pd_reg_type == VIO_STA)
> > + reg += 0x4 * index;
> > +
> > + } else {
> > + pr_err(PFX "%s:0x%x or %s:0x%x or %s:0x%x is out of boundary\n",
> > + "slave_type", slave_type,
>
> Move "slave_type" into format string.
Why is this necessary? Is there any benefit for moving this?
Since the line length is almost over 80 chars.
>
> > + "pd_reg_type", pd_reg_type,
> > + "index", index);
> > + return NULL;
> > + }
> > +
> > + return reg;
> > +}
> > +
>
[snip]
>
> > +
> > +/*
> > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > + * violation information including which master violates
> > + * access slave.
> > + */
> > +static irqreturn_t devapc_violation_irq(int irq_number, void *dev_id)
> > +{
> > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > + const struct mtk_device_info **device_info;
> > + struct mtk_devapc_vio_info *vio_info;
> > + int slave_type, vio_idx, index;
> > + const char *vio_master;
> > + unsigned long flags;
> > + bool normal;
> > + u8 perm;
> > +
> > + spin_lock_irqsave(&devapc_lock, flags);
> > +
> > + device_info = mtk_devapc_ctx->soc->device_info;
> > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > + normal = false;
> > + vio_idx = -1;
> > + index = -1;
> > +
> > + /* There are multiple DEVAPC_PD */
> > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > + if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> > + if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> > + &index))
> > + continue;
> > +
> > + /* Ensure that violation info are written before
> > + * further operations
> > + */
> > + smp_mb();
> > + normal = true;
> > +
> > + mask_module_irq(slave_type, vio_idx, true);
> > +
> > + if (clear_vio_status(slave_type, vio_idx))
> > + pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> > + "clear vio status failed",
> > + "slave_type", slave_type,
> > + "vio_index", vio_idx);
> > +
> > + perm = get_permission(slave_type, index, vio_info->domain_id);
> > +
> > + vio_master = mtk_devapc_ctx->soc->master_get
> > + (vio_info->master_id,
> > + vio_info->vio_addr,
> > + slave_type,
> > + vio_info->shift_sta_bit,
> > + vio_info->domain_id);
>
> Call mt6873_bus_id_to_master() directly. For first patch, make things
> as simple as possible.
In devapc_violation_irq() function, we use common flow to handle each
devapc violation on different platforms. The master_get() has different
implementation on different platforms, that why it called indirectly.
Once we have new platform, we only have to update devapc-mtxxxx.c
instead of common handler flow.
>
> > +
> > + if (!vio_master) {
> > + pr_warn(PFX "master_get failed\n");
> > + vio_master = "UNKNOWN_MASTER";
> > + }
> > +
> > + pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> > + "Violation", "slave_type", slave_type,
> > + "sys_index",
> > + device_info[slave_type][index].sys_index,
> > + "ctrl_index",
> > + device_info[slave_type][index].ctrl_index,
> > + "vio_index",
> > + device_info[slave_type][index].vio_index);
> > +
> > + pr_info(PFX "%s %s %s %s\n",
> > + "Violation - master:", vio_master,
> > + "access violation slave:",
> > + device_info[slave_type][index].device);
> > +
> > + devapc_vio_reason(perm);
> > +
> > + devapc_extra_handler(slave_type, vio_master, vio_idx,
> > + vio_info->vio_addr);
> > +
> > + mask_module_irq(slave_type, vio_idx, false);
> > + }
> > +
> > + if (normal) {
> > + spin_unlock_irqrestore(&devapc_lock, flags);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + spin_unlock_irqrestore(&devapc_lock, flags);
> > + return IRQ_HANDLED;
> > +}
> > +
[snip]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-11 9:26 ` Neal Liu
@ 2020-06-11 11:01 ` Chun-Kuang Hu
2020-06-12 3:04 ` Neal Liu
0 siblings, 1 reply; 20+ messages in thread
From: Chun-Kuang Hu @ 2020-06-11 11:01 UTC (permalink / raw)
To: Neal Liu
Cc: Chun-Kuang Hu, wsd_upstream, devicetree, linux-kernel,
Rob Herring, moderated list:ARM/Mediatek SoC support,
Matthias Brugger, Linux ARM
Hi, Neal:
Neal Liu <neal.liu@mediatek.com> 於 2020年6月11日 週四 下午5:26寫道：
>
> On Wed, 2020-06-10 at 00:01 +0800, Chun-Kuang Hu wrote:
> Hi Chun-Kuang,
>
> [snip]
>
> > > +
> > > +/*
> > > + * mtk_devapc_pd_get - get devapc pd_types of register address.
> > > + *
> > > + * Returns the value of reg addr
> > > + */
> > > +static void __iomem *mtk_devapc_pd_get(int slave_type,
> > > + enum DEVAPC_PD_REG_TYPE pd_reg_type,
> > > + u32 index)
> > > +{
> > > + struct mtk_devapc_vio_info *vio_info = mtk_devapc_ctx->soc->vio_info;
> > > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > > + const u32 *devapc_pds = mtk_devapc_ctx->soc->devapc_pds;
> > > + void __iomem *reg;
> > > +
> > > + if (!devapc_pds)
> > > + return NULL;
> > > +
> > > + if ((slave_type < slave_type_num &&
> > > + index < vio_info->vio_mask_sta_num[slave_type]) &&
> > > + pd_reg_type < PD_REG_TYPE_NUM) {
> > > + reg = mtk_devapc_ctx->devapc_pd_base[slave_type] +
> > > + devapc_pds[pd_reg_type];
> > > +
> > > + if (pd_reg_type == VIO_MASK || pd_reg_type == VIO_STA)
> > > + reg += 0x4 * index;
> > > +
> > > + } else {
> > > + pr_err(PFX "%s:0x%x or %s:0x%x or %s:0x%x is out of boundary\n",
> > > + "slave_type", slave_type,
> >
> > Move "slave_type" into format string.
>
> Why is this necessary? Is there any benefit for moving this?
Smaller code size, simple, intuition.
> Since the line length is almost over 80 chars.
Single string could be over 80 chars.
>
> >
> > > + "pd_reg_type", pd_reg_type,
> > > + "index", index);
> > > + return NULL;
> > > + }
> > > +
> > > + return reg;
> > > +}
> > > +
> >
>
> [snip]
>
> >
> > > +
> > > +/*
> > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > + * violation information including which master violates
> > > + * access slave.
> > > + */
> > > +static irqreturn_t devapc_violation_irq(int irq_number, void *dev_id)
> > > +{
> > > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > > + const struct mtk_device_info **device_info;
> > > + struct mtk_devapc_vio_info *vio_info;
> > > + int slave_type, vio_idx, index;
> > > + const char *vio_master;
> > > + unsigned long flags;
> > > + bool normal;
> > > + u8 perm;
> > > +
> > > + spin_lock_irqsave(&devapc_lock, flags);
> > > +
> > > + device_info = mtk_devapc_ctx->soc->device_info;
> > > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > > + normal = false;
> > > + vio_idx = -1;
> > > + index = -1;
> > > +
> > > + /* There are multiple DEVAPC_PD */
> > > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > > + if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> > > + if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> > > + &index))
> > > + continue;
> > > +
> > > + /* Ensure that violation info are written before
> > > + * further operations
> > > + */
> > > + smp_mb();
> > > + normal = true;
> > > +
> > > + mask_module_irq(slave_type, vio_idx, true);
> > > +
> > > + if (clear_vio_status(slave_type, vio_idx))
> > > + pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> > > + "clear vio status failed",
> > > + "slave_type", slave_type,
> > > + "vio_index", vio_idx);
> > > +
> > > + perm = get_permission(slave_type, index, vio_info->domain_id);
> > > +
> > > + vio_master = mtk_devapc_ctx->soc->master_get
> > > + (vio_info->master_id,
> > > + vio_info->vio_addr,
> > > + slave_type,
> > > + vio_info->shift_sta_bit,
> > > + vio_info->domain_id);
> >
> > Call mt6873_bus_id_to_master() directly. For first patch, make things
> > as simple as possible.
>
> In devapc_violation_irq() function, we use common flow to handle each
> devapc violation on different platforms. The master_get() has different
> implementation on different platforms, that why it called indirectly.
>
> Once we have new platform, we only have to update devapc-mtxxxx.c
> instead of common handler flow.
You just upstream one SoC now, so I have no information of 2nd SoC.
Without the 2nd SoC, how do we know what is common and what is SoC special?
So the first patch should not consider the things which does not exist yet.
Regards,
Chun-Kuang.
>
> >
> > > +
> > > + if (!vio_master) {
> > > + pr_warn(PFX "master_get failed\n");
> > > + vio_master = "UNKNOWN_MASTER";
> > > + }
> > > +
> > > + pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> > > + "Violation", "slave_type", slave_type,
> > > + "sys_index",
> > > + device_info[slave_type][index].sys_index,
> > > + "ctrl_index",
> > > + device_info[slave_type][index].ctrl_index,
> > > + "vio_index",
> > > + device_info[slave_type][index].vio_index);
> > > +
> > > + pr_info(PFX "%s %s %s %s\n",
> > > + "Violation - master:", vio_master,
> > > + "access violation slave:",
> > > + device_info[slave_type][index].device);
> > > +
> > > + devapc_vio_reason(perm);
> > > +
> > > + devapc_extra_handler(slave_type, vio_master, vio_idx,
> > > + vio_info->vio_addr);
> > > +
> > > + mask_module_irq(slave_type, vio_idx, false);
> > > + }
> > > +
> > > + if (normal) {
> > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > + return IRQ_HANDLED;
> > > + }
> > > +
> > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > + return IRQ_HANDLED;
> > > +}
> > > +
>
> [snip]
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-11 11:01 ` Chun-Kuang Hu
@ 2020-06-12 3:04 ` Neal Liu
2020-06-12 15:27 ` Chun-Kuang Hu
0 siblings, 1 reply; 20+ messages in thread
From: Neal Liu @ 2020-06-12 3:04 UTC (permalink / raw)
To: Chun-Kuang Hu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Neal Liu, Linux ARM
Hi Chun-Kuang,
[snip]
> > > > +/*
> > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > + * violation information including which master violates
> > > > + * access slave.
> > > > + */
> > > > +static irqreturn_t devapc_violation_irq(int irq_number, void *dev_id)
> > > > +{
> > > > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > > > + const struct mtk_device_info **device_info;
> > > > + struct mtk_devapc_vio_info *vio_info;
> > > > + int slave_type, vio_idx, index;
> > > > + const char *vio_master;
> > > > + unsigned long flags;
> > > > + bool normal;
> > > > + u8 perm;
> > > > +
> > > > + spin_lock_irqsave(&devapc_lock, flags);
> > > > +
> > > > + device_info = mtk_devapc_ctx->soc->device_info;
> > > > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > > > + normal = false;
> > > > + vio_idx = -1;
> > > > + index = -1;
> > > > +
> > > > + /* There are multiple DEVAPC_PD */
> > > > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > > > + if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> > > > + if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> > > > + &index))
> > > > + continue;
> > > > +
> > > > + /* Ensure that violation info are written before
> > > > + * further operations
> > > > + */
> > > > + smp_mb();
> > > > + normal = true;
> > > > +
> > > > + mask_module_irq(slave_type, vio_idx, true);
> > > > +
> > > > + if (clear_vio_status(slave_type, vio_idx))
> > > > + pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> > > > + "clear vio status failed",
> > > > + "slave_type", slave_type,
> > > > + "vio_index", vio_idx);
> > > > +
> > > > + perm = get_permission(slave_type, index, vio_info->domain_id);
> > > > +
> > > > + vio_master = mtk_devapc_ctx->soc->master_get
> > > > + (vio_info->master_id,
> > > > + vio_info->vio_addr,
> > > > + slave_type,
> > > > + vio_info->shift_sta_bit,
> > > > + vio_info->domain_id);
> > >
> > > Call mt6873_bus_id_to_master() directly. For first patch, make things
> > > as simple as possible.
> >
> > In devapc_violation_irq() function, we use common flow to handle each
> > devapc violation on different platforms. The master_get() has different
> > implementation on different platforms, that why it called indirectly.
> >
> > Once we have new platform, we only have to update devapc-mtxxxx.c
> > instead of common handler flow.
>
> You just upstream one SoC now, so I have no information of 2nd SoC.
> Without the 2nd SoC, how do we know what is common and what is SoC special?
> So the first patch should not consider the things which does not exist yet.
>
> Regards,
> Chun-Kuang.
>
It has lots of refactoring work need to do if you really want make it
"simple". Could I explain more details and let you judge it is simple
enough?
For most MediaTek DEVAPC hw, the violation interrupt handling sequence
is shown below.
1. Domain processor receives a interrupt issued by DEVAPC.
2. Software read the violation status and identify it.
3. Software read the debug information which are stored in hw register.
a. debug information includes master ID, domain ID, violation
address, ...
4. Transfer debug information to human readable strings.
5. Extra handler to dispatch owner directly.
What we really care is which master violates the rules, and which slave
had been accessed unexpectedly.
Here are platform specific information:
1. Slaves layout (platform devices)
2. hw register layout which are stored violation information
3. Master ID mapping table
4. Domain ID mapping table
Hope these steps could help you understand what is common and what is
SoC specific. If you want to see the 2nd SoC's driver, I can also send
it for you to take a look.
Thanks,
Neal
> >
> > >
> > > > +
> > > > + if (!vio_master) {
> > > > + pr_warn(PFX "master_get failed\n");
> > > > + vio_master = "UNKNOWN_MASTER";
> > > > + }
> > > > +
> > > > + pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> > > > + "Violation", "slave_type", slave_type,
> > > > + "sys_index",
> > > > + device_info[slave_type][index].sys_index,
> > > > + "ctrl_index",
> > > > + device_info[slave_type][index].ctrl_index,
> > > > + "vio_index",
> > > > + device_info[slave_type][index].vio_index);
> > > > +
> > > > + pr_info(PFX "%s %s %s %s\n",
> > > > + "Violation - master:", vio_master,
> > > > + "access violation slave:",
> > > > + device_info[slave_type][index].device);
> > > > +
> > > > + devapc_vio_reason(perm);
> > > > +
> > > > + devapc_extra_handler(slave_type, vio_master, vio_idx,
> > > > + vio_info->vio_addr);
> > > > +
> > > > + mask_module_irq(slave_type, vio_idx, false);
> > > > + }
> > > > +
> > > > + if (normal) {
> > > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > > + return IRQ_HANDLED;
> > > > + }
> > > > +
> > > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> >
> > [snip]
> >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-12 3:04 ` Neal Liu
@ 2020-06-12 15:27 ` Chun-Kuang Hu
2020-06-15 2:12 ` Neal Liu
0 siblings, 1 reply; 20+ messages in thread
From: Chun-Kuang Hu @ 2020-06-12 15:27 UTC (permalink / raw)
To: Neal Liu
Cc: Chun-Kuang Hu, wsd_upstream, devicetree, linux-kernel,
Rob Herring, moderated list:ARM/Mediatek SoC support,
Matthias Brugger, Linux ARM
Hi, Neal:
Neal Liu <neal.liu@mediatek.com> 於 2020年6月12日 週五 上午11:04寫道：
>
> Hi Chun-Kuang,
>
> [snip]
> > > > > +/*
> > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > + * violation information including which master violates
> > > > > + * access slave.
> > > > > + */
> > > > > +static irqreturn_t devapc_violation_irq(int irq_number, void *dev_id)
> > > > > +{
> > > > > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > > > > + const struct mtk_device_info **device_info;
> > > > > + struct mtk_devapc_vio_info *vio_info;
> > > > > + int slave_type, vio_idx, index;
> > > > > + const char *vio_master;
> > > > > + unsigned long flags;
> > > > > + bool normal;
> > > > > + u8 perm;
> > > > > +
> > > > > + spin_lock_irqsave(&devapc_lock, flags);
> > > > > +
> > > > > + device_info = mtk_devapc_ctx->soc->device_info;
> > > > > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > > > > + normal = false;
> > > > > + vio_idx = -1;
> > > > > + index = -1;
> > > > > +
> > > > > + /* There are multiple DEVAPC_PD */
> > > > > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > > > > + if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> > > > > + if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> > > > > + &index))
> > > > > + continue;
> > > > > +
> > > > > + /* Ensure that violation info are written before
> > > > > + * further operations
> > > > > + */
> > > > > + smp_mb();
> > > > > + normal = true;
> > > > > +
> > > > > + mask_module_irq(slave_type, vio_idx, true);
> > > > > +
> > > > > + if (clear_vio_status(slave_type, vio_idx))
> > > > > + pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> > > > > + "clear vio status failed",
> > > > > + "slave_type", slave_type,
> > > > > + "vio_index", vio_idx);
> > > > > +
> > > > > + perm = get_permission(slave_type, index, vio_info->domain_id);
> > > > > +
> > > > > + vio_master = mtk_devapc_ctx->soc->master_get
> > > > > + (vio_info->master_id,
> > > > > + vio_info->vio_addr,
> > > > > + slave_type,
> > > > > + vio_info->shift_sta_bit,
> > > > > + vio_info->domain_id);
> > > >
> > > > Call mt6873_bus_id_to_master() directly. For first patch, make things
> > > > as simple as possible.
> > >
> > > In devapc_violation_irq() function, we use common flow to handle each
> > > devapc violation on different platforms. The master_get() has different
> > > implementation on different platforms, that why it called indirectly.
> > >
> > > Once we have new platform, we only have to update devapc-mtxxxx.c
> > > instead of common handler flow.
> >
> > You just upstream one SoC now, so I have no information of 2nd SoC.
> > Without the 2nd SoC, how do we know what is common and what is SoC special?
> > So the first patch should not consider the things which does not exist yet.
> >
> > Regards,
> > Chun-Kuang.
> >
>
> It has lots of refactoring work need to do if you really want make it
> "simple". Could I explain more details and let you judge it is simple
> enough?
Making driver "simple" is very important, so it worth to spend effort
to make things simple. Everybody could modify this driver, so make
this driver simple and everybody would join this easily.
> For most MediaTek DEVAPC hw, the violation interrupt handling sequence
> is shown below.
>
> 1. Domain processor receives a interrupt issued by DEVAPC.
> 2. Software read the violation status and identify it.
> 3. Software read the debug information which are stored in hw register.
> a. debug information includes master ID, domain ID, violation
> address, ...
> 4. Transfer debug information to human readable strings.
> 5. Extra handler to dispatch owner directly.
I don't know why need extra handler? What does this extra handler could do?
If indeed need it, separate extra handler part to an independent patch.
>
> What we really care is which master violates the rules, and which slave
> had been accessed unexpectedly.
>
> Here are platform specific information:
> 1. Slaves layout (platform devices)
> 2. hw register layout which are stored violation information
> 3. Master ID mapping table
> 4. Domain ID mapping table
>
> Hope these steps could help you understand what is common and what is
> SoC specific. If you want to see the 2nd SoC's driver, I can also send
> it for you to take a look.
Please upstream 2nd SoC's driver, so I could review common part and
SoC specific part.
Regards,
Chun-Kuang.
>
> Thanks,
> Neal
>
> > >
> > > >
> > > > > +
> > > > > + if (!vio_master) {
> > > > > + pr_warn(PFX "master_get failed\n");
> > > > > + vio_master = "UNKNOWN_MASTER";
> > > > > + }
> > > > > +
> > > > > + pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> > > > > + "Violation", "slave_type", slave_type,
> > > > > + "sys_index",
> > > > > + device_info[slave_type][index].sys_index,
> > > > > + "ctrl_index",
> > > > > + device_info[slave_type][index].ctrl_index,
> > > > > + "vio_index",
> > > > > + device_info[slave_type][index].vio_index);
> > > > > +
> > > > > + pr_info(PFX "%s %s %s %s\n",
> > > > > + "Violation - master:", vio_master,
> > > > > + "access violation slave:",
> > > > > + device_info[slave_type][index].device);
> > > > > +
> > > > > + devapc_vio_reason(perm);
> > > > > +
> > > > > + devapc_extra_handler(slave_type, vio_master, vio_idx,
> > > > > + vio_info->vio_addr);
> > > > > +
> > > > > + mask_module_irq(slave_type, vio_idx, false);
> > > > > + }
> > > > > +
> > > > > + if (normal) {
> > > > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > > > + return IRQ_HANDLED;
> > > > > + }
> > > > > +
> > > > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > > > + return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > >
> > > [snip]
> > >
> > >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
[not found] ` <1591698261-22639-3-git-send-email-neal.liu@mediatek.com>
2020-06-09 16:01 ` [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver Chun-Kuang Hu
@ 2020-06-12 23:20 ` Chun-Kuang Hu
2020-06-16 6:45 ` Neal Liu
2020-06-14 3:26 ` Chun-Kuang Hu
2020-06-15 15:51 ` Chun-Kuang Hu
3 siblings, 1 reply; 20+ messages in thread
From: Chun-Kuang Hu @ 2020-06-12 23:20 UTC (permalink / raw)
To: Neal Liu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Linux ARM
Hi, Neal:
Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
>
> MT6873 bus frabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violations are 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 devapc-mt6873 driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>
[snip]
> +
> +/*
> + * sramrom_vio_handler - clean sramrom violation & print violation information
> + * for debugging.
> + */
> +static void sramrom_vio_handler(void)
> +{
> + const struct mtk_sramrom_sec_vio_desc *sramrom_vios;
> + struct mtk_devapc_vio_info *vio_info;
> + struct arm_smccc_res res;
> + size_t sramrom_vio_sta;
> + int sramrom_vio;
> + u32 rw;
> +
> + sramrom_vios = mtk_devapc_ctx->soc->sramrom_sec_vios;
> + vio_info = mtk_devapc_ctx->soc->vio_info;
> +
> + arm_smccc_smc(MTK_SIP_KERNEL_CLR_SRAMROM_VIO,
> + 0, 0, 0, 0, 0, 0, 0, &res);
> +
> + sramrom_vio = res.a0;
> + sramrom_vio_sta = res.a1;
> + vio_info->vio_addr = res.a2;
> +
> + if (sramrom_vio == SRAM_VIOLATION)
> + pr_info(PFX "%s, SRAM violation is triggered\n", __func__);
> + else if (sramrom_vio == ROM_VIOLATION)
> + pr_info(PFX "%s, ROM violation is triggered\n", __func__);
> + else
> + return;
> +
> + vio_info->master_id = (sramrom_vio_sta & sramrom_vios->vio_id_mask)
> + >> sramrom_vios->vio_id_shift;
> + vio_info->domain_id = (sramrom_vio_sta & sramrom_vios->vio_domain_mask)
> + >> sramrom_vios->vio_domain_shift;
> + rw = (sramrom_vio_sta & sramrom_vios->vio_rw_mask) >>
> + sramrom_vios->vio_rw_shift;
I think some information, such as master_id, would be get in
devapc_extract_vio_dbg(), you need not to get it here.
> +
> + if (rw)
> + vio_info->write = 1;
> + else
> + vio_info->read = 1;
> +
> + pr_info(PFX "%s: %s:0x%x, %s:0x%x, %s:%s, %s:0x%x\n",
> + __func__, "master_id", vio_info->master_id,
> + "domain_id", vio_info->domain_id,
> + "rw", rw ? "Write" : "Read",
> + "vio_addr", vio_info->vio_addr);
> +}
> +
[snip]
> +
> +/*
> + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> + * violation information including which master violates
> + * access slave.
> + */
> +static irqreturn_t devapc_violation_irq(int irq_number, void *dev_id)
> +{
> + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
Don't make mtk_devapc_ctx a global variable. You should allocate
instance of mtk_devapc_ctx in probe(), and pass mtk_devapc_ctx to
the last parameter of devm_request_irq(), and it would be the second
parameter of irq handler.
> + const struct mtk_device_info **device_info;
> + struct mtk_devapc_vio_info *vio_info;
> + int slave_type, vio_idx, index;
> + const char *vio_master;
> + unsigned long flags;
> + bool normal;
> + u8 perm;
> +
> + spin_lock_irqsave(&devapc_lock, flags);
> +
> + device_info = mtk_devapc_ctx->soc->device_info;
> + vio_info = mtk_devapc_ctx->soc->vio_info;
> + normal = false;
> + vio_idx = -1;
> + index = -1;
> +
> + /* There are multiple DEVAPC_PD */
> + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> + if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> + if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> + &index))
> + continue;
> +
> + /* Ensure that violation info are written before
> + * further operations
> + */
> + smp_mb();
> + normal = true;
> +
> + mask_module_irq(slave_type, vio_idx, true);
> +
> + if (clear_vio_status(slave_type, vio_idx))
> + pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> + "clear vio status failed",
> + "slave_type", slave_type,
> + "vio_index", vio_idx);
> +
> + perm = get_permission(slave_type, index, vio_info->domain_id);
> +
> + vio_master = mtk_devapc_ctx->soc->master_get
> + (vio_info->master_id,
> + vio_info->vio_addr,
> + slave_type,
> + vio_info->shift_sta_bit,
> + vio_info->domain_id);
> +
> + if (!vio_master) {
> + pr_warn(PFX "master_get failed\n");
> + vio_master = "UNKNOWN_MASTER";
> + }
> +
> + pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> + "Violation", "slave_type", slave_type,
> + "sys_index",
> + device_info[slave_type][index].sys_index,
> + "ctrl_index",
> + device_info[slave_type][index].ctrl_index,
> + "vio_index",
> + device_info[slave_type][index].vio_index);
> +
> + pr_info(PFX "%s %s %s %s\n",
> + "Violation - master:", vio_master,
> + "access violation slave:",
> + device_info[slave_type][index].device);
> +
> + devapc_vio_reason(perm);
> +
> + devapc_extra_handler(slave_type, vio_master, vio_idx,
> + vio_info->vio_addr);
> +
> + mask_module_irq(slave_type, vio_idx, false);
> + }
> +
> + if (normal) {
> + spin_unlock_irqrestore(&devapc_lock, flags);
> + return IRQ_HANDLED;
> + }
> +
> + spin_unlock_irqrestore(&devapc_lock, flags);
> + return IRQ_HANDLED;
> +}
> +
[snip]
> +
> +int mtk_devapc_probe(struct platform_device *pdev, struct mtk_devapc_soc *soc)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + u32 slave_type_num;
> + int slave_type;
> + int ret;
> +
> + if (IS_ERR(node))
> + return -ENODEV;
> +
> + mtk_devapc_ctx->soc = soc;
> + slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> +
> + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> + mtk_devapc_ctx->devapc_pd_base[slave_type] =
> + of_iomap(node, slave_type);
> + if (!mtk_devapc_ctx->devapc_pd_base[slave_type])
> + return -EINVAL;
> + }
> +
> + mtk_devapc_ctx->infracfg_base = of_iomap(node, slave_type_num + 1);
> + if (!mtk_devapc_ctx->infracfg_base)
> + return -EINVAL;
> +
> + mtk_devapc_ctx->devapc_irq = irq_of_parse_and_map(node, 0);
> + if (!mtk_devapc_ctx->devapc_irq)
> + return -EINVAL;
> +
> + /* CCF (Common Clock Framework) */
> + mtk_devapc_ctx->devapc_infra_clk = devm_clk_get(&pdev->dev,
> + "devapc-infra-clock");
> +
> + if (IS_ERR(mtk_devapc_ctx->devapc_infra_clk))
> + return -EINVAL;
> +
> + proc_create("devapc_dbg", 0664, NULL, &devapc_dbg_fops);
> +
> + if (clk_prepare_enable(mtk_devapc_ctx->devapc_infra_clk))
> + return -EINVAL;
> +
> + start_devapc();
> +
> + ret = devm_request_irq(&pdev->dev, mtk_devapc_ctx->devapc_irq,
> + (irq_handler_t)devapc_violation_irq,
> + IRQF_TRIGGER_NONE, "devapc", NULL);
> + if (ret) {
> + pr_err(PFX "request devapc irq failed, ret:%d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(mtk_devapc_probe);
Why export probe function?
> +
> +int mtk_devapc_remove(struct platform_device *dev)
> +{
> + clk_disable_unprepare(mtk_devapc_ctx->devapc_infra_clk);
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(mtk_devapc_remove);
Ditto.
Regards,
Chun-Kuang.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
[not found] ` <1591698261-22639-3-git-send-email-neal.liu@mediatek.com>
2020-06-09 16:01 ` [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver Chun-Kuang Hu
2020-06-12 23:20 ` Chun-Kuang Hu
@ 2020-06-14 3:26 ` Chun-Kuang Hu
2020-06-15 2:43 ` Neal Liu
2020-06-15 15:51 ` Chun-Kuang Hu
3 siblings, 1 reply; 20+ messages in thread
From: Chun-Kuang Hu @ 2020-06-14 3:26 UTC (permalink / raw)
To: Neal Liu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Linux ARM
Hi, Neal:
Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
>
> MT6873 bus frabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violations are 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 devapc-mt6873 driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> ---
[snip]
> +
> + /* 50 */
> + {-1, -1, 50, "OOB_way_en", true},
> + {-1, -1, 51, "OOB_way_en", true},
> + {-1, -1, 52, "OOB_way_en", true},
> + {-1, -1, 53, "OOB_way_en", true},
> + {-1, -1, 54, "OOB_way_en", true},
> + {-1, -1, 55, "OOB_way_en", true},
> + {-1, -1, 56, "Decode_error", true},
> + {-1, -1, 57, "Decode_error", true},
> + {-1, -1, 58, "DISP_PWM", false},
> + {-1, -1, 59, "IMP_IIC_WRAP", false},
> +
> + /* 60 */
> + {-1, -1, 60, "DEVICE_APC_PERI_PAR__AO", false},
> + {-1, -1, 61, "DEVICE_APC_PERI_PAR_PDN", false},
You does not process the item whose enable_vio_irq is false, so I
think you should remove these items and remove enable_vio_irq because
the rest item's enable_vio_irq would always be true.
> +};
> +
> +static struct mtk_device_num mtk6873_devices_num[] = {
> + {SLAVE_TYPE_INFRA, VIO_SLAVE_NUM_INFRA},
> + {SLAVE_TYPE_PERI, VIO_SLAVE_NUM_PERI},
> + {SLAVE_TYPE_PERI2, VIO_SLAVE_NUM_PERI2},
> + {SLAVE_TYPE_PERI_PAR, VIO_SLAVE_NUM_PERI_PAR},
> +};
> +
> +static struct PERIAXI_ID_INFO peri_mi_id_to_master[] = {
> + {"THERM2", { 0, 0, 0 } },
> + {"SPM", { 0, 1, 0 } },
> + {"CCU", { 0, 0, 1 } },
> + {"THERM", { 0, 1, 1 } },
> + {"SPM_DRAMC", { 1, 1, 0 } },
The bits { 1, 1, 0 } equal to a number 0x3, I thiink you should use a
number instead of bits and everything would be more easy.
> +};
> +
[snip]
> +
> +/*
> + * mtk_devapc_vio_check - check violation shift status is raised or not.
> + *
> + * Returns the value of violation shift status reg
> + */
> +static void mtk_devapc_vio_check(int slave_type, int *shift_bit)
> +{
> + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> + struct mtk_devapc_vio_info *vio_info;
> + u32 vio_shift_sta;
> + int i;
> +
> + if (slave_type >= slave_type_num) {
This never happen, so remove it.
> + pr_err(PFX "%s: param check failed, %s:0x%x\n",
> + __func__, "slave_type", slave_type);
> + return;
> + }
> +
> + vio_info = mtk_devapc_ctx->soc->vio_info;
> + vio_shift_sta = readl(mtk_devapc_pd_get(slave_type, VIO_SHIFT_STA, 0));
> +
> + if (!vio_shift_sta) {
> + pr_info(PFX "violation is triggered before. %s:0x%x\n",
> + "shift_bit", *shift_bit);
> +
> + } else if (vio_shift_sta & (0x1UL << *shift_bit)) {
> + pr_debug(PFX "%s: 0x%x is matched with %s:%d\n",
> + "vio_shift_sta", vio_shift_sta,
> + "shift_bit", *shift_bit);
> +
> + } else {
> + pr_info(PFX "%s: 0x%x is not matched with %s:%d\n",
> + "vio_shift_sta", vio_shift_sta,
> + "shift_bit", *shift_bit);
> +
> + for (i = 0; i < MOD_NO_IN_1_DEVAPC * 2; i++) {
> + if (vio_shift_sta & (0x1 << i)) {
> + *shift_bit = i;
> + break;
> + }
> + }
> + }
> +
> + vio_info->shift_sta_bit = *shift_bit;
> +}
> +
> +static void devapc_extract_vio_dbg(int slave_type)
> +{
> + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> + void __iomem *vio_dbg0_reg, *vio_dbg1_reg, *vio_dbg2_reg;
> + const struct mtk_infra_vio_dbg_desc *vio_dbgs;
> + struct mtk_devapc_vio_info *vio_info;
> + u32 dbg0;
> +
> + if (slave_type >= slave_type_num) {
Ditto.
Regards,
Chun-Kuang.
> + pr_err(PFX "%s: param check failed, %s:0x%x\n",
> + __func__, "slave_type", slave_type);
> + return;
> + }
> +
> + vio_dbg0_reg = mtk_devapc_pd_get(slave_type, VIO_DBG0, 0);
> + vio_dbg1_reg = mtk_devapc_pd_get(slave_type, VIO_DBG1, 0);
> + vio_dbg2_reg = mtk_devapc_pd_get(slave_type, VIO_DBG2, 0);
> +
> + vio_dbgs = mtk_devapc_ctx->soc->vio_dbgs;
> + vio_info = mtk_devapc_ctx->soc->vio_info;
> +
> + /* Extract violation information */
> + dbg0 = readl(vio_dbg0_reg);
> + vio_info->master_id = readl(vio_dbg1_reg);
> + vio_info->vio_addr = readl(vio_dbg2_reg);
> +
> + vio_info->domain_id = (dbg0 & vio_dbgs->vio_dbg_dmnid)
> + >> vio_dbgs->vio_dbg_dmnid_start_bit;
> + vio_info->write = ((dbg0 & vio_dbgs->vio_dbg_w_vio)
> + >> vio_dbgs->vio_dbg_w_vio_start_bit) == 1;
> + vio_info->read = ((dbg0 & vio_dbgs->vio_dbg_r_vio)
> + >> vio_dbgs->vio_dbg_r_vio_start_bit) == 1;
> + vio_info->vio_addr_high = (dbg0 & vio_dbgs->vio_addr_high)
> + >> vio_dbgs->vio_addr_high_start_bit;
> +
> + devapc_vio_info_print();
> +}
> +
> +/*
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-12 15:27 ` Chun-Kuang Hu
@ 2020-06-15 2:12 ` Neal Liu
0 siblings, 0 replies; 20+ messages in thread
From: Neal Liu @ 2020-06-15 2:12 UTC (permalink / raw)
To: Chun-Kuang Hu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Linux ARM
Hi Chun-Kuang,
On Fri, 2020-06-12 at 23:27 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu <neal.liu@mediatek.com> 於 2020年6月12日 週五 上午11:04寫道：
> >
> > Hi Chun-Kuang,
> >
> > [snip]
> > > > > > +/*
> > > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > > + * violation information including which master violates
> > > > > > + * access slave.
> > > > > > + */
> > > > > > +static irqreturn_t devapc_violation_irq(int irq_number, void *dev_id)
> > > > > > +{
> > > > > > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > > > > > + const struct mtk_device_info **device_info;
> > > > > > + struct mtk_devapc_vio_info *vio_info;
> > > > > > + int slave_type, vio_idx, index;
> > > > > > + const char *vio_master;
> > > > > > + unsigned long flags;
> > > > > > + bool normal;
> > > > > > + u8 perm;
> > > > > > +
> > > > > > + spin_lock_irqsave(&devapc_lock, flags);
> > > > > > +
> > > > > > + device_info = mtk_devapc_ctx->soc->device_info;
> > > > > > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > > > > > + normal = false;
> > > > > > + vio_idx = -1;
> > > > > > + index = -1;
> > > > > > +
> > > > > > + /* There are multiple DEVAPC_PD */
> > > > > > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > > > > > + if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> > > > > > + if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> > > > > > + &index))
> > > > > > + continue;
> > > > > > +
> > > > > > + /* Ensure that violation info are written before
> > > > > > + * further operations
> > > > > > + */
> > > > > > + smp_mb();
> > > > > > + normal = true;
> > > > > > +
> > > > > > + mask_module_irq(slave_type, vio_idx, true);
> > > > > > +
> > > > > > + if (clear_vio_status(slave_type, vio_idx))
> > > > > > + pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> > > > > > + "clear vio status failed",
> > > > > > + "slave_type", slave_type,
> > > > > > + "vio_index", vio_idx);
> > > > > > +
> > > > > > + perm = get_permission(slave_type, index, vio_info->domain_id);
> > > > > > +
> > > > > > + vio_master = mtk_devapc_ctx->soc->master_get
> > > > > > + (vio_info->master_id,
> > > > > > + vio_info->vio_addr,
> > > > > > + slave_type,
> > > > > > + vio_info->shift_sta_bit,
> > > > > > + vio_info->domain_id);
> > > > >
> > > > > Call mt6873_bus_id_to_master() directly. For first patch, make things
> > > > > as simple as possible.
> > > >
> > > > In devapc_violation_irq() function, we use common flow to handle each
> > > > devapc violation on different platforms. The master_get() has different
> > > > implementation on different platforms, that why it called indirectly.
> > > >
> > > > Once we have new platform, we only have to update devapc-mtxxxx.c
> > > > instead of common handler flow.
> > >
> > > You just upstream one SoC now, so I have no information of 2nd SoC.
> > > Without the 2nd SoC, how do we know what is common and what is SoC special?
> > > So the first patch should not consider the things which does not exist yet.
> > >
> > > Regards,
> > > Chun-Kuang.
> > >
> >
> > It has lots of refactoring work need to do if you really want make it
> > "simple". Could I explain more details and let you judge it is simple
> > enough?
>
> Making driver "simple" is very important, so it worth to spend effort
> to make things simple. Everybody could modify this driver, so make
> this driver simple and everybody would join this easily.
>
> > For most MediaTek DEVAPC hw, the violation interrupt handling sequence
> > is shown below.
> >
> > 1. Domain processor receives a interrupt issued by DEVAPC.
> > 2. Software read the violation status and identify it.
> > 3. Software read the debug information which are stored in hw register.
> > a. debug information includes master ID, domain ID, violation
> > address, ...
> > 4. Transfer debug information to human readable strings.
> > 5. Extra handler to dispatch owner directly.
>
> I don't know why need extra handler? What does this extra handler could do?
> If indeed need it, separate extra handler part to an independent patch.
Yes, You are right. I'll make this driver more simple and let everybody
can join it easily. This extra handler may not be necessary for whom try
to enable this driver.
> >
> > What we really care is which master violates the rules, and which slave
> > had been accessed unexpectedly.
> >
> > Here are platform specific information:
> > 1. Slaves layout (platform devices)
> > 2. hw register layout which are stored violation information
> > 3. Master ID mapping table
> > 4. Domain ID mapping table
> >
> > Hope these steps could help you understand what is common and what is
> > SoC specific. If you want to see the 2nd SoC's driver, I can also send
> > it for you to take a look.
>
> Please upstream 2nd SoC's driver, so I could review common part and
> SoC specific part.
>
> Regards,
> Chun-Kuang.
>
> >
> > Thanks,
> > Neal
> >
> > > >
> > > > >
> > > > > > +
> > > > > > + if (!vio_master) {
> > > > > > + pr_warn(PFX "master_get failed\n");
> > > > > > + vio_master = "UNKNOWN_MASTER";
> > > > > > + }
> > > > > > +
> > > > > > + pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> > > > > > + "Violation", "slave_type", slave_type,
> > > > > > + "sys_index",
> > > > > > + device_info[slave_type][index].sys_index,
> > > > > > + "ctrl_index",
> > > > > > + device_info[slave_type][index].ctrl_index,
> > > > > > + "vio_index",
> > > > > > + device_info[slave_type][index].vio_index);
> > > > > > +
> > > > > > + pr_info(PFX "%s %s %s %s\n",
> > > > > > + "Violation - master:", vio_master,
> > > > > > + "access violation slave:",
> > > > > > + device_info[slave_type][index].device);
> > > > > > +
> > > > > > + devapc_vio_reason(perm);
> > > > > > +
> > > > > > + devapc_extra_handler(slave_type, vio_master, vio_idx,
> > > > > > + vio_info->vio_addr);
> > > > > > +
> > > > > > + mask_module_irq(slave_type, vio_idx, false);
> > > > > > + }
> > > > > > +
> > > > > > + if (normal) {
> > > > > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > > > > + return IRQ_HANDLED;
> > > > > > + }
> > > > > > +
> > > > > > + spin_unlock_irqrestore(&devapc_lock, flags);
> > > > > > + return IRQ_HANDLED;
> > > > > > +}
> > > > > > +
> > > >
> > > > [snip]
> > > >
> > > >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-14 3:26 ` Chun-Kuang Hu
@ 2020-06-15 2:43 ` Neal Liu
2020-06-15 14:14 ` Chun-Kuang Hu
0 siblings, 1 reply; 20+ messages in thread
From: Neal Liu @ 2020-06-15 2:43 UTC (permalink / raw)
To: Chun-Kuang Hu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring, Neal Liu,
Matthias Brugger, moderated list:ARM/Mediatek SoC support,
Linux ARM
Hi Chun-Kuang,
On Sun, 2020-06-14 at 11:26 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
> >
> > MT6873 bus frabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violations are 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 devapc-mt6873 driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > ---
>
> [snip]
>
> > +
> > + /* 50 */
> > + {-1, -1, 50, "OOB_way_en", true},
> > + {-1, -1, 51, "OOB_way_en", true},
> > + {-1, -1, 52, "OOB_way_en", true},
> > + {-1, -1, 53, "OOB_way_en", true},
> > + {-1, -1, 54, "OOB_way_en", true},
> > + {-1, -1, 55, "OOB_way_en", true},
> > + {-1, -1, 56, "Decode_error", true},
> > + {-1, -1, 57, "Decode_error", true},
> > + {-1, -1, 58, "DISP_PWM", false},
> > + {-1, -1, 59, "IMP_IIC_WRAP", false},
> > +
> > + /* 60 */
> > + {-1, -1, 60, "DEVICE_APC_PERI_PAR__AO", false},
> > + {-1, -1, 61, "DEVICE_APC_PERI_PAR_PDN", false},
>
> You does not process the item whose enable_vio_irq is false, so I
> think you should remove these items and remove enable_vio_irq because
> the rest item's enable_vio_irq would always be true.
In some users, they can decide which slaves they want to enable or
disable violation irq in different product. We remain this property for
compatibility.
>
> > +};
> > +
> > +static struct mtk_device_num mtk6873_devices_num[] = {
> > + {SLAVE_TYPE_INFRA, VIO_SLAVE_NUM_INFRA},
> > + {SLAVE_TYPE_PERI, VIO_SLAVE_NUM_PERI},
> > + {SLAVE_TYPE_PERI2, VIO_SLAVE_NUM_PERI2},
> > + {SLAVE_TYPE_PERI_PAR, VIO_SLAVE_NUM_PERI_PAR},
> > +};
> > +
> > +static struct PERIAXI_ID_INFO peri_mi_id_to_master[] = {
> > + {"THERM2", { 0, 0, 0 } },
> > + {"SPM", { 0, 1, 0 } },
> > + {"CCU", { 0, 0, 1 } },
> > + {"THERM", { 0, 1, 1 } },
> > + {"SPM_DRAMC", { 1, 1, 0 } },
>
> The bits { 1, 1, 0 } equal to a number 0x3, I thiink you should use a
> number instead of bits and everything would be more easy.
We would like to keep it because the bit value contains more than 0/1,
it might be '2' in some cases. '2' means it can be 0 or 1. This totally
by hardware design & implementation.
> > +};
> > +
>
> [snip]
>
> > +
> > +/*
> > + * mtk_devapc_vio_check - check violation shift status is raised or not.
> > + *
> > + * Returns the value of violation shift status reg
> > + */
> > +static void mtk_devapc_vio_check(int slave_type, int *shift_bit)
> > +{
> > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > + struct mtk_devapc_vio_info *vio_info;
> > + u32 vio_shift_sta;
> > + int i;
> > +
> > + if (slave_type >= slave_type_num) {
>
> This never happen, so remove it.
Indeed, thanks
>
> > + pr_err(PFX "%s: param check failed, %s:0x%x\n",
> > + __func__, "slave_type", slave_type);
> > + return;
> > + }
> > +
> > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > + vio_shift_sta = readl(mtk_devapc_pd_get(slave_type, VIO_SHIFT_STA, 0));
> > +
> > + if (!vio_shift_sta) {
> > + pr_info(PFX "violation is triggered before. %s:0x%x\n",
> > + "shift_bit", *shift_bit);
> > +
> > + } else if (vio_shift_sta & (0x1UL << *shift_bit)) {
> > + pr_debug(PFX "%s: 0x%x is matched with %s:%d\n",
> > + "vio_shift_sta", vio_shift_sta,
> > + "shift_bit", *shift_bit);
> > +
> > + } else {
> > + pr_info(PFX "%s: 0x%x is not matched with %s:%d\n",
> > + "vio_shift_sta", vio_shift_sta,
> > + "shift_bit", *shift_bit);
> > +
> > + for (i = 0; i < MOD_NO_IN_1_DEVAPC * 2; i++) {
> > + if (vio_shift_sta & (0x1 << i)) {
> > + *shift_bit = i;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + vio_info->shift_sta_bit = *shift_bit;
> > +}
> > +
> > +static void devapc_extract_vio_dbg(int slave_type)
> > +{
> > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > + void __iomem *vio_dbg0_reg, *vio_dbg1_reg, *vio_dbg2_reg;
> > + const struct mtk_infra_vio_dbg_desc *vio_dbgs;
> > + struct mtk_devapc_vio_info *vio_info;
> > + u32 dbg0;
> > +
> > + if (slave_type >= slave_type_num) {
>
> Ditto.
Indeed, thanks
>
> Regards,
> Chun-Kuang.
>
> > + pr_err(PFX "%s: param check failed, %s:0x%x\n",
> > + __func__, "slave_type", slave_type);
> > + return;
> > + }
> > +
> > + vio_dbg0_reg = mtk_devapc_pd_get(slave_type, VIO_DBG0, 0);
> > + vio_dbg1_reg = mtk_devapc_pd_get(slave_type, VIO_DBG1, 0);
> > + vio_dbg2_reg = mtk_devapc_pd_get(slave_type, VIO_DBG2, 0);
> > +
> > + vio_dbgs = mtk_devapc_ctx->soc->vio_dbgs;
> > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > +
> > + /* Extract violation information */
> > + dbg0 = readl(vio_dbg0_reg);
> > + vio_info->master_id = readl(vio_dbg1_reg);
> > + vio_info->vio_addr = readl(vio_dbg2_reg);
> > +
> > + vio_info->domain_id = (dbg0 & vio_dbgs->vio_dbg_dmnid)
> > + >> vio_dbgs->vio_dbg_dmnid_start_bit;
> > + vio_info->write = ((dbg0 & vio_dbgs->vio_dbg_w_vio)
> > + >> vio_dbgs->vio_dbg_w_vio_start_bit) == 1;
> > + vio_info->read = ((dbg0 & vio_dbgs->vio_dbg_r_vio)
> > + >> vio_dbgs->vio_dbg_r_vio_start_bit) == 1;
> > + vio_info->vio_addr_high = (dbg0 & vio_dbgs->vio_addr_high)
> > + >> vio_dbgs->vio_addr_high_start_bit;
> > +
> > + devapc_vio_info_print();
> > +}
> > +
> > +/*
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-15 2:43 ` Neal Liu
@ 2020-06-15 14:14 ` Chun-Kuang Hu
2020-06-15 14:17 ` Chun-Kuang Hu
0 siblings, 1 reply; 20+ messages in thread
From: Chun-Kuang Hu @ 2020-06-15 14:14 UTC (permalink / raw)
To: Neal Liu
Cc: Chun-Kuang Hu, wsd_upstream, devicetree, linux-kernel,
Rob Herring, moderated list:ARM/Mediatek SoC support,
Matthias Brugger, Linux ARM
Hi, Neal:
Neal Liu <neal.liu@mediatek.com> 於 2020年6月15日 週一 上午10:43寫道：
>
> Hi Chun-Kuang,
>
>
> On Sun, 2020-06-14 at 11:26 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
> > >
> > > MT6873 bus frabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violations are 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 devapc-mt6873 driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > ---
> >
> > [snip]
> >
> > > +
> > > + /* 50 */
> > > + {-1, -1, 50, "OOB_way_en", true},
> > > + {-1, -1, 51, "OOB_way_en", true},
> > > + {-1, -1, 52, "OOB_way_en", true},
> > > + {-1, -1, 53, "OOB_way_en", true},
> > > + {-1, -1, 54, "OOB_way_en", true},
> > > + {-1, -1, 55, "OOB_way_en", true},
> > > + {-1, -1, 56, "Decode_error", true},
> > > + {-1, -1, 57, "Decode_error", true},
> > > + {-1, -1, 58, "DISP_PWM", false},
> > > + {-1, -1, 59, "IMP_IIC_WRAP", false},
> > > +
> > > + /* 60 */
> > > + {-1, -1, 60, "DEVICE_APC_PERI_PAR__AO", false},
> > > + {-1, -1, 61, "DEVICE_APC_PERI_PAR_PDN", false},
> >
> > You does not process the item whose enable_vio_irq is false, so I
> > think you should remove these items and remove enable_vio_irq because
> > the rest item's enable_vio_irq would always be true.
>
> In some users, they can decide which slaves they want to enable or
> disable violation irq in different product. We remain this property for
> compatibility.
I think in upstream version, you could still remove enable_vio_irq and
process all items. For downstream case, users could remove items they
does not interest in.
>
> >
> > > +};
> > > +
> > > +static struct mtk_device_num mtk6873_devices_num[] = {
> > > + {SLAVE_TYPE_INFRA, VIO_SLAVE_NUM_INFRA},
> > > + {SLAVE_TYPE_PERI, VIO_SLAVE_NUM_PERI},
> > > + {SLAVE_TYPE_PERI2, VIO_SLAVE_NUM_PERI2},
> > > + {SLAVE_TYPE_PERI_PAR, VIO_SLAVE_NUM_PERI_PAR},
> > > +};
> > > +
> > > +static struct PERIAXI_ID_INFO peri_mi_id_to_master[] = {
> > > + {"THERM2", { 0, 0, 0 } },
> > > + {"SPM", { 0, 1, 0 } },
> > > + {"CCU", { 0, 0, 1 } },
> > > + {"THERM", { 0, 1, 1 } },
> > > + {"SPM_DRAMC", { 1, 1, 0 } },
> >
> > The bits { 1, 1, 0 } equal to a number 0x3, I thiink you should use a
> > number instead of bits and everything would be more easy.
>
> We would like to keep it because the bit value contains more than 0/1,
> it might be '2' in some cases. '2' means it can be 0 or 1. This totally
> by hardware design & implementation.
Upstream the patch that has dont-care-bits, otherwise, use a number for this.
Even there is dont-care-bits, I have a better way to process it. Here
is an example that has dont-care-bits:
+ {"Tinysys", { 0, 1, 0, 0, 2, 2, 2, 2, 2, 2, 0, 0, 0, 0 } },
I could use a { value, mask } pair for this case,
value = 0x0002; /* value for care bits */
mask = 0x3c02; /* mask for care bits */
So the compare statement would be
if ((bus_id & mask) == value)
So you could get rid of the second for-loop and reduce the processing
time in irq handler.
Regards,
Chun-Kuang.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-15 14:14 ` Chun-Kuang Hu
@ 2020-06-15 14:17 ` Chun-Kuang Hu
2020-06-16 6:09 ` Neal Liu
0 siblings, 1 reply; 20+ messages in thread
From: Chun-Kuang Hu @ 2020-06-15 14:17 UTC (permalink / raw)
To: Chun-Kuang Hu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring, Neal Liu,
Matthias Brugger, moderated list:ARM/Mediatek SoC support,
Linux ARM
Chun-Kuang Hu <chunkuang.hu@kernel.org> 於 2020年6月15日 週一 下午10:14寫道：
>
> Hi, Neal:
>
> Neal Liu <neal.liu@mediatek.com> 於 2020年6月15日 週一 上午10:43寫道：
> >
> > Hi Chun-Kuang,
> >
> >
> > On Sun, 2020-06-14 at 11:26 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
> > > >
> > > > MT6873 bus frabric provides TrustZone security support and data
> > > > protection to prevent slaves from being accessed by unexpected
> > > > masters.
> > > > The security violations are 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 devapc-mt6873 driver. The violation
> > > > information is printed in order to find the murderer.
> > > >
> > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > > ---
> > >
> > > [snip]
> > >
> > > > +
> > > > + /* 50 */
> > > > + {-1, -1, 50, "OOB_way_en", true},
> > > > + {-1, -1, 51, "OOB_way_en", true},
> > > > + {-1, -1, 52, "OOB_way_en", true},
> > > > + {-1, -1, 53, "OOB_way_en", true},
> > > > + {-1, -1, 54, "OOB_way_en", true},
> > > > + {-1, -1, 55, "OOB_way_en", true},
> > > > + {-1, -1, 56, "Decode_error", true},
> > > > + {-1, -1, 57, "Decode_error", true},
> > > > + {-1, -1, 58, "DISP_PWM", false},
> > > > + {-1, -1, 59, "IMP_IIC_WRAP", false},
> > > > +
> > > > + /* 60 */
> > > > + {-1, -1, 60, "DEVICE_APC_PERI_PAR__AO", false},
> > > > + {-1, -1, 61, "DEVICE_APC_PERI_PAR_PDN", false},
> > >
> > > You does not process the item whose enable_vio_irq is false, so I
> > > think you should remove these items and remove enable_vio_irq because
> > > the rest item's enable_vio_irq would always be true.
> >
> > In some users, they can decide which slaves they want to enable or
> > disable violation irq in different product. We remain this property for
> > compatibility.
>
> I think in upstream version, you could still remove enable_vio_irq and
> process all items. For downstream case, users could remove items they
> does not interest in.
>
> >
> > >
> > > > +};
> > > > +
> > > > +static struct mtk_device_num mtk6873_devices_num[] = {
> > > > + {SLAVE_TYPE_INFRA, VIO_SLAVE_NUM_INFRA},
> > > > + {SLAVE_TYPE_PERI, VIO_SLAVE_NUM_PERI},
> > > > + {SLAVE_TYPE_PERI2, VIO_SLAVE_NUM_PERI2},
> > > > + {SLAVE_TYPE_PERI_PAR, VIO_SLAVE_NUM_PERI_PAR},
> > > > +};
> > > > +
> > > > +static struct PERIAXI_ID_INFO peri_mi_id_to_master[] = {
> > > > + {"THERM2", { 0, 0, 0 } },
> > > > + {"SPM", { 0, 1, 0 } },
> > > > + {"CCU", { 0, 0, 1 } },
> > > > + {"THERM", { 0, 1, 1 } },
> > > > + {"SPM_DRAMC", { 1, 1, 0 } },
> > >
> > > The bits { 1, 1, 0 } equal to a number 0x3, I thiink you should use a
> > > number instead of bits and everything would be more easy.
> >
> > We would like to keep it because the bit value contains more than 0/1,
> > it might be '2' in some cases. '2' means it can be 0 or 1. This totally
> > by hardware design & implementation.
>
> Upstream the patch that has dont-care-bits, otherwise, use a number for this.
> Even there is dont-care-bits, I have a better way to process it. Here
> is an example that has dont-care-bits:
>
> + {"Tinysys", { 0, 1, 0, 0, 2, 2, 2, 2, 2, 2, 0, 0, 0, 0 } },
>
> I could use a { value, mask } pair for this case,
>
> value = 0x0002; /* value for care bits */
> mask = 0x3c02; /* mask for care bits */
Sorry, this would be
mask = 0x3c0f; /* mask for care bits */
>
> So the compare statement would be
>
> if ((bus_id & mask) == value)
>
> So you could get rid of the second for-loop and reduce the processing
> time in irq handler.
>
> Regards,
> Chun-Kuang.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
[not found] ` <1591698261-22639-3-git-send-email-neal.liu@mediatek.com>
` (2 preceding siblings ...)
2020-06-14 3:26 ` Chun-Kuang Hu
@ 2020-06-15 15:51 ` Chun-Kuang Hu
2020-06-16 6:19 ` Neal Liu
3 siblings, 1 reply; 20+ messages in thread
From: Chun-Kuang Hu @ 2020-06-15 15:51 UTC (permalink / raw)
To: Neal Liu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Linux ARM
Hi, Neal:
Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
>
> MT6873 bus frabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violations are 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 devapc-mt6873 driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> ---
[snip]
> + {1, 0, 22, "MMSYS", true},
> + {1, 1, 23, "MMSYS_DISP", true},
> + {1, 2, 24, "SMI", true},
> + {1, 3, 25, "SMI", true},
> + {1, 4, 26, "SMI", true},
> + {1, 5, 27, "MMSYS_DISP", true},
> + {1, 6, 28, "MMSYS_DISP", true},
> +
> + /* 30 */
> + {1, 7, 29, "MMSYS_DISP", true},
> + {1, 8, 30, "MMSYS_DISP", true},
> + {1, 9, 31, "MMSYS_DISP", true},
> + {1, 10, 32, "MMSYS_DISP", true},
> + {1, 11, 33, "MMSYS_DISP", true},
> + {1, 12, 34, "MMSYS_DISP", true},
> + {1, 13, 35, "MMSYS_DISP", true},
> + {1, 14, 36, "MMSYS_DISP", true},
> + {1, 15, 37, "MMSYS_DISP", true},
> + {1, 16, 38, "MMSYS_DISP", true},
> +
> + /* 40 */
> + {1, 17, 39, "MMSYS_DISP", true},
> + {1, 18, 40, "MMSYS_DISP", true},
> + {1, 19, 41, "MMSYS_DISP", true},
> + {1, 20, 42, "MMSYS_DISP", true},
> + {1, 21, 43, "MMSYS_DISP", true},
> + {1, 22, 44, "MMSYS_DISP", true},
I think the device name, such as "MMSYS_DISP" does not help for debug.
When DevAPC print "MMSYS_DISP" has error, how does us know that to do
next? WHERE is the code may related to this error, and WHO should us
to find help? I think we just need vio address. Using mt8173 for
example, if the vio address is 0x1400d03c, we could find the device in
mt8173.dtsi [1],
ovl1: ovl@1400d000 {
compatible = "mediatek,mt8173-disp-ovl";
reg = <0 0x1400d000 0 0x1000>;
};
we could know error occur in ovl1, and we could find the compatible
string "mediatek,mt8173-disp-ovl" in
drivers/gpu/drm/mediatek/mtk_drm_drv.c, so we know WHERE is the code
may related to this error. And we could find maintainer list [2] to
find out the maintainer of this code:
DRM DRIVERS FOR MEDIATEK
M: Chun-Kuang Hu <chunkuang.hu@kernel.org>
M: Philipp Zabel <p.zabel@pengutronix.de>
L: dri-devel@lists.freedesktop.org
S: Supported
F: Documentation/devicetree/bindings/display/mediatek/
F: drivers/gpu/drm/mediatek/
and we know find WHO for help.
So I think we should drop device name and just print vio address is
enough for debug.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.8-rc1
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS?h=v5.8-rc1
> + {1, 23, 45, "MMSYS_MDP", true},
> + {1, 24, 46, "MMSYS_MDP", true},
> + {1, 25, 47, "MMSYS_MDP", true},
> + {1, 26, 48, "MMSYS_MDP", true},
> +
[snip]
> +
> +int mtk_devapc_probe(struct platform_device *pdev, struct mtk_devapc_soc *soc)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + u32 slave_type_num;
> + int slave_type;
> + int ret;
> +
> + if (IS_ERR(node))
> + return -ENODEV;
> +
> + mtk_devapc_ctx->soc = soc;
> + slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> +
> + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> + mtk_devapc_ctx->devapc_pd_base[slave_type] =
> + of_iomap(node, slave_type);
> + if (!mtk_devapc_ctx->devapc_pd_base[slave_type])
> + return -EINVAL;
> + }
> +
> + mtk_devapc_ctx->infracfg_base = of_iomap(node, slave_type_num + 1);
> + if (!mtk_devapc_ctx->infracfg_base)
> + return -EINVAL;
> +
> + mtk_devapc_ctx->devapc_irq = irq_of_parse_and_map(node, 0);
> + if (!mtk_devapc_ctx->devapc_irq)
> + return -EINVAL;
> +
> + /* CCF (Common Clock Framework) */
> + mtk_devapc_ctx->devapc_infra_clk = devm_clk_get(&pdev->dev,
> + "devapc-infra-clock");
> +
> + if (IS_ERR(mtk_devapc_ctx->devapc_infra_clk))
> + return -EINVAL;
> +
> + proc_create("devapc_dbg", 0664, NULL, &devapc_dbg_fops);
I think debugfs is not a basic function, so move debugfs function to
another patch.
Regards,
Chun-Kuang.
> +
> + if (clk_prepare_enable(mtk_devapc_ctx->devapc_infra_clk))
> + return -EINVAL;
> +
> + start_devapc();
> +
> + ret = devm_request_irq(&pdev->dev, mtk_devapc_ctx->devapc_irq,
> + (irq_handler_t)devapc_violation_irq,
> + IRQF_TRIGGER_NONE, "devapc", NULL);
> + if (ret) {
> + pr_err(PFX "request devapc irq failed, ret:%d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(mtk_devapc_probe);
> +
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-15 14:17 ` Chun-Kuang Hu
@ 2020-06-16 6:09 ` Neal Liu
0 siblings, 0 replies; 20+ messages in thread
From: Neal Liu @ 2020-06-16 6:09 UTC (permalink / raw)
To: Chun-Kuang Hu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring, Neal Liu,
Matthias Brugger, moderated list:ARM/Mediatek SoC support,
Linux ARM
Hi Chun-Kuang,
On Mon, 2020-06-15 at 22:17 +0800, Chun-Kuang Hu wrote:
> Chun-Kuang Hu <chunkuang.hu@kernel.org> 於 2020年6月15日 週一 下午10:14寫道：
> >
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@mediatek.com> 於 2020年6月15日 週一 上午10:43寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > >
> > > On Sun, 2020-06-14 at 11:26 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
> > > > >
> > > > > MT6873 bus frabric provides TrustZone security support and data
> > > > > protection to prevent slaves from being accessed by unexpected
> > > > > masters.
> > > > > The security violations are 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 devapc-mt6873 driver. The violation
> > > > > information is printed in order to find the murderer.
> > > > >
> > > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > > > > ---
> > > >
> > > > [snip]
> > > >
> > > > > +
> > > > > + /* 50 */
> > > > > + {-1, -1, 50, "OOB_way_en", true},
> > > > > + {-1, -1, 51, "OOB_way_en", true},
> > > > > + {-1, -1, 52, "OOB_way_en", true},
> > > > > + {-1, -1, 53, "OOB_way_en", true},
> > > > > + {-1, -1, 54, "OOB_way_en", true},
> > > > > + {-1, -1, 55, "OOB_way_en", true},
> > > > > + {-1, -1, 56, "Decode_error", true},
> > > > > + {-1, -1, 57, "Decode_error", true},
> > > > > + {-1, -1, 58, "DISP_PWM", false},
> > > > > + {-1, -1, 59, "IMP_IIC_WRAP", false},
> > > > > +
> > > > > + /* 60 */
> > > > > + {-1, -1, 60, "DEVICE_APC_PERI_PAR__AO", false},
> > > > > + {-1, -1, 61, "DEVICE_APC_PERI_PAR_PDN", false},
> > > >
> > > > You does not process the item whose enable_vio_irq is false, so I
> > > > think you should remove these items and remove enable_vio_irq because
> > > > the rest item's enable_vio_irq would always be true.
> > >
> > > In some users, they can decide which slaves they want to enable or
> > > disable violation irq in different product. We remain this property for
> > > compatibility.
> >
> > I think in upstream version, you could still remove enable_vio_irq and
> > process all items. For downstream case, users could remove items they
> > does not interest in.
Okay, sounds good. I'll try to revise and upstream again.
> >
> > >
> > > >
> > > > > +};
> > > > > +
> > > > > +static struct mtk_device_num mtk6873_devices_num[] = {
> > > > > + {SLAVE_TYPE_INFRA, VIO_SLAVE_NUM_INFRA},
> > > > > + {SLAVE_TYPE_PERI, VIO_SLAVE_NUM_PERI},
> > > > > + {SLAVE_TYPE_PERI2, VIO_SLAVE_NUM_PERI2},
> > > > > + {SLAVE_TYPE_PERI_PAR, VIO_SLAVE_NUM_PERI_PAR},
> > > > > +};
> > > > > +
> > > > > +static struct PERIAXI_ID_INFO peri_mi_id_to_master[] = {
> > > > > + {"THERM2", { 0, 0, 0 } },
> > > > > + {"SPM", { 0, 1, 0 } },
> > > > > + {"CCU", { 0, 0, 1 } },
> > > > > + {"THERM", { 0, 1, 1 } },
> > > > > + {"SPM_DRAMC", { 1, 1, 0 } },
> > > >
> > > > The bits { 1, 1, 0 } equal to a number 0x3, I thiink you should use a
> > > > number instead of bits and everything would be more easy.
> > >
> > > We would like to keep it because the bit value contains more than 0/1,
> > > it might be '2' in some cases. '2' means it can be 0 or 1. This totally
> > > by hardware design & implementation.
> >
> > Upstream the patch that has dont-care-bits, otherwise, use a number for this.
> > Even there is dont-care-bits, I have a better way to process it. Here
> > is an example that has dont-care-bits:
> >
> > + {"Tinysys", { 0, 1, 0, 0, 2, 2, 2, 2, 2, 2, 0, 0, 0, 0 } },
> >
> > I could use a { value, mask } pair for this case,
> >
> > value = 0x0002; /* value for care bits */
> > mask = 0x3c02; /* mask for care bits */
>
> Sorry, this would be
>
> mask = 0x3c0f; /* mask for care bits */
>
> >
> > So the compare statement would be
> >
> > if ((bus_id & mask) == value)
> >
> > So you could get rid of the second for-loop and reduce the processing
> > time in irq handler.
> >
Great idea! I'll try to revise and upstream again.
> > Regards,
> > Chun-Kuang.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-15 15:51 ` Chun-Kuang Hu
@ 2020-06-16 6:19 ` Neal Liu
0 siblings, 0 replies; 20+ messages in thread
From: Neal Liu @ 2020-06-16 6:19 UTC (permalink / raw)
To: Chun-Kuang Hu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring, Neal Liu,
Matthias Brugger, moderated list:ARM/Mediatek SoC support,
Linux ARM
Hi Chun-Kuang,
On Mon, 2020-06-15 at 23:51 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
> >
> > MT6873 bus frabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violations are 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 devapc-mt6873 driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > ---
>
> [snip]
>
> > + {1, 0, 22, "MMSYS", true},
> > + {1, 1, 23, "MMSYS_DISP", true},
> > + {1, 2, 24, "SMI", true},
> > + {1, 3, 25, "SMI", true},
> > + {1, 4, 26, "SMI", true},
> > + {1, 5, 27, "MMSYS_DISP", true},
> > + {1, 6, 28, "MMSYS_DISP", true},
> > +
> > + /* 30 */
> > + {1, 7, 29, "MMSYS_DISP", true},
> > + {1, 8, 30, "MMSYS_DISP", true},
> > + {1, 9, 31, "MMSYS_DISP", true},
> > + {1, 10, 32, "MMSYS_DISP", true},
> > + {1, 11, 33, "MMSYS_DISP", true},
> > + {1, 12, 34, "MMSYS_DISP", true},
> > + {1, 13, 35, "MMSYS_DISP", true},
> > + {1, 14, 36, "MMSYS_DISP", true},
> > + {1, 15, 37, "MMSYS_DISP", true},
> > + {1, 16, 38, "MMSYS_DISP", true},
> > +
> > + /* 40 */
> > + {1, 17, 39, "MMSYS_DISP", true},
> > + {1, 18, 40, "MMSYS_DISP", true},
> > + {1, 19, 41, "MMSYS_DISP", true},
> > + {1, 20, 42, "MMSYS_DISP", true},
> > + {1, 21, 43, "MMSYS_DISP", true},
> > + {1, 22, 44, "MMSYS_DISP", true},
>
> I think the device name, such as "MMSYS_DISP" does not help for debug.
> When DevAPC print "MMSYS_DISP" has error, how does us know that to do
> next? WHERE is the code may related to this error, and WHO should us
> to find help? I think we just need vio address. Using mt8173 for
> example, if the vio address is 0x1400d03c, we could find the device in
> mt8173.dtsi [1],
>
> ovl1: ovl@1400d000 {
> compatible = "mediatek,mt8173-disp-ovl";
> reg = <0 0x1400d000 0 0x1000>;
> };
>
> we could know error occur in ovl1, and we could find the compatible
> string "mediatek,mt8173-disp-ovl" in
> drivers/gpu/drm/mediatek/mtk_drm_drv.c, so we know WHERE is the code
> may related to this error. And we could find maintainer list [2] to
> find out the maintainer of this code:
>
> DRM DRIVERS FOR MEDIATEK
> M: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> M: Philipp Zabel <p.zabel@pengutronix.de>
> L: dri-devel@lists.freedesktop.org
> S: Supported
> F: Documentation/devicetree/bindings/display/mediatek/
> F: drivers/gpu/drm/mediatek/
>
> and we know find WHO for help.
> So I think we should drop device name and just print vio address is
> enough for debug.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.8-rc1
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS?h=v5.8-rc1
>
You are right. This is a way to find WHO can help this issue.
We integrated our internal debug system with device name, and it can
help us to find module owner automatically instead of searching dts and
source code manually.
But this kinds of debugging flow is not necessary for upstream. I'll try
to remove it.
> > + {1, 23, 45, "MMSYS_MDP", true},
> > + {1, 24, 46, "MMSYS_MDP", true},
> > + {1, 25, 47, "MMSYS_MDP", true},
> > + {1, 26, 48, "MMSYS_MDP", true},
> > +
>
> [snip]
>
> > +
> > +int mtk_devapc_probe(struct platform_device *pdev, struct mtk_devapc_soc *soc)
> > +{
> > + struct device_node *node = pdev->dev.of_node;
> > + u32 slave_type_num;
> > + int slave_type;
> > + int ret;
> > +
> > + if (IS_ERR(node))
> > + return -ENODEV;
> > +
> > + mtk_devapc_ctx->soc = soc;
> > + slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > +
> > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > + mtk_devapc_ctx->devapc_pd_base[slave_type] =
> > + of_iomap(node, slave_type);
> > + if (!mtk_devapc_ctx->devapc_pd_base[slave_type])
> > + return -EINVAL;
> > + }
> > +
> > + mtk_devapc_ctx->infracfg_base = of_iomap(node, slave_type_num + 1);
> > + if (!mtk_devapc_ctx->infracfg_base)
> > + return -EINVAL;
> > +
> > + mtk_devapc_ctx->devapc_irq = irq_of_parse_and_map(node, 0);
> > + if (!mtk_devapc_ctx->devapc_irq)
> > + return -EINVAL;
> > +
> > + /* CCF (Common Clock Framework) */
> > + mtk_devapc_ctx->devapc_infra_clk = devm_clk_get(&pdev->dev,
> > + "devapc-infra-clock");
> > +
> > + if (IS_ERR(mtk_devapc_ctx->devapc_infra_clk))
> > + return -EINVAL;
> > +
> > + proc_create("devapc_dbg", 0664, NULL, &devapc_dbg_fops);
>
> I think debugfs is not a basic function, so move debugfs function to
> another patch.
>
Okay, I'll try to remove and upstream again.
> Regards,
> Chun-Kuang.
>
> > +
> > + if (clk_prepare_enable(mtk_devapc_ctx->devapc_infra_clk))
> > + return -EINVAL;
> > +
> > + start_devapc();
> > +
> > + ret = devm_request_irq(&pdev->dev, mtk_devapc_ctx->devapc_irq,
> > + (irq_handler_t)devapc_violation_irq,
> > + IRQF_TRIGGER_NONE, "devapc", NULL);
> > + if (ret) {
> > + pr_err(PFX "request devapc irq failed, ret:%d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_devapc_probe);
> > +
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver
2020-06-12 23:20 ` Chun-Kuang Hu
@ 2020-06-16 6:45 ` Neal Liu
0 siblings, 0 replies; 20+ messages in thread
From: Neal Liu @ 2020-06-16 6:45 UTC (permalink / raw)
To: Chun-Kuang Hu
Cc: devicetree, wsd_upstream, linux-kernel, Rob Herring,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Neal Liu, Linux ARM
Hi Chun-Kuang,
On Sat, 2020-06-13 at 07:20 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
> >
> > MT6873 bus frabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violations are 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 devapc-mt6873 driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
>
> [snip]
>
> > +
> > +/*
> > + * sramrom_vio_handler - clean sramrom violation & print violation information
> > + * for debugging.
> > + */
> > +static void sramrom_vio_handler(void)
> > +{
> > + const struct mtk_sramrom_sec_vio_desc *sramrom_vios;
> > + struct mtk_devapc_vio_info *vio_info;
> > + struct arm_smccc_res res;
> > + size_t sramrom_vio_sta;
> > + int sramrom_vio;
> > + u32 rw;
> > +
> > + sramrom_vios = mtk_devapc_ctx->soc->sramrom_sec_vios;
> > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > +
> > + arm_smccc_smc(MTK_SIP_KERNEL_CLR_SRAMROM_VIO,
> > + 0, 0, 0, 0, 0, 0, 0, &res);
> > +
> > + sramrom_vio = res.a0;
> > + sramrom_vio_sta = res.a1;
> > + vio_info->vio_addr = res.a2;
> > +
> > + if (sramrom_vio == SRAM_VIOLATION)
> > + pr_info(PFX "%s, SRAM violation is triggered\n", __func__);
> > + else if (sramrom_vio == ROM_VIOLATION)
> > + pr_info(PFX "%s, ROM violation is triggered\n", __func__);
> > + else
> > + return;
> > +
> > + vio_info->master_id = (sramrom_vio_sta & sramrom_vios->vio_id_mask)
> > + >> sramrom_vios->vio_id_shift;
> > + vio_info->domain_id = (sramrom_vio_sta & sramrom_vios->vio_domain_mask)
> > + >> sramrom_vios->vio_domain_shift;
> > + rw = (sramrom_vio_sta & sramrom_vios->vio_rw_mask) >>
> > + sramrom_vios->vio_rw_shift;
>
> I think some information, such as master_id, would be get in
> devapc_extract_vio_dbg(), you need not to get it here.
sramrom violation is from type2 slaves, and these kinds of slaves'
violation informations are stored in different registers than normal
slaves.
So we would check is it type2 violation or normal violation, and get
violation information from corresponding registers.
> > +
> > + if (rw)
> > + vio_info->write = 1;
> > + else
> > + vio_info->read = 1;
> > +
> > + pr_info(PFX "%s: %s:0x%x, %s:0x%x, %s:%s, %s:0x%x\n",
> > + __func__, "master_id", vio_info->master_id,
> > + "domain_id", vio_info->domain_id,
> > + "rw", rw ? "Write" : "Read",
> > + "vio_addr", vio_info->vio_addr);
> > +}
> > +
>
> [snip]
>
> > +
> > +/*
> > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > + * violation information including which master violates
> > + * access slave.
> > + */
> > +static irqreturn_t devapc_violation_irq(int irq_number, void *dev_id)
> > +{
> > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
>
> Don't make mtk_devapc_ctx a global variable. You should allocate
> instance of mtk_devapc_ctx in probe(), and pass mtk_devapc_ctx to
> the last parameter of devm_request_irq(), and it would be the second
> parameter of irq handler.
Okay, I'll try to revise and upstream again.
> > + const struct mtk_device_info **device_info;
> > + struct mtk_devapc_vio_info *vio_info;
> > + int slave_type, vio_idx, index;
> > + const char *vio_master;
> > + unsigned long flags;
> > + bool normal;
> > + u8 perm;
> > +
> > + spin_lock_irqsave(&devapc_lock, flags);
> > +
> > + device_info = mtk_devapc_ctx->soc->device_info;
> > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > + normal = false;
> > + vio_idx = -1;
> > + index = -1;
> > +
> > + /* There are multiple DEVAPC_PD */
> > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > + if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> > + if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> > + &index))
> > + continue;
> > +
> > + /* Ensure that violation info are written before
> > + * further operations
> > + */
> > + smp_mb();
> > + normal = true;
> > +
> > + mask_module_irq(slave_type, vio_idx, true);
> > +
> > + if (clear_vio_status(slave_type, vio_idx))
> > + pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> > + "clear vio status failed",
> > + "slave_type", slave_type,
> > + "vio_index", vio_idx);
> > +
> > + perm = get_permission(slave_type, index, vio_info->domain_id);
> > +
> > + vio_master = mtk_devapc_ctx->soc->master_get
> > + (vio_info->master_id,
> > + vio_info->vio_addr,
> > + slave_type,
> > + vio_info->shift_sta_bit,
> > + vio_info->domain_id);
> > +
> > + if (!vio_master) {
> > + pr_warn(PFX "master_get failed\n");
> > + vio_master = "UNKNOWN_MASTER";
> > + }
> > +
> > + pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> > + "Violation", "slave_type", slave_type,
> > + "sys_index",
> > + device_info[slave_type][index].sys_index,
> > + "ctrl_index",
> > + device_info[slave_type][index].ctrl_index,
> > + "vio_index",
> > + device_info[slave_type][index].vio_index);
> > +
> > + pr_info(PFX "%s %s %s %s\n",
> > + "Violation - master:", vio_master,
> > + "access violation slave:",
> > + device_info[slave_type][index].device);
> > +
> > + devapc_vio_reason(perm);
> > +
> > + devapc_extra_handler(slave_type, vio_master, vio_idx,
> > + vio_info->vio_addr);
> > +
> > + mask_module_irq(slave_type, vio_idx, false);
> > + }
> > +
> > + if (normal) {
> > + spin_unlock_irqrestore(&devapc_lock, flags);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + spin_unlock_irqrestore(&devapc_lock, flags);
> > + return IRQ_HANDLED;
> > +}
> > +
>
> [snip]
>
> > +
> > +int mtk_devapc_probe(struct platform_device *pdev, struct mtk_devapc_soc *soc)
> > +{
> > + struct device_node *node = pdev->dev.of_node;
> > + u32 slave_type_num;
> > + int slave_type;
> > + int ret;
> > +
> > + if (IS_ERR(node))
> > + return -ENODEV;
> > +
> > + mtk_devapc_ctx->soc = soc;
> > + slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > +
> > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > + mtk_devapc_ctx->devapc_pd_base[slave_type] =
> > + of_iomap(node, slave_type);
> > + if (!mtk_devapc_ctx->devapc_pd_base[slave_type])
> > + return -EINVAL;
> > + }
> > +
> > + mtk_devapc_ctx->infracfg_base = of_iomap(node, slave_type_num + 1);
> > + if (!mtk_devapc_ctx->infracfg_base)
> > + return -EINVAL;
> > +
> > + mtk_devapc_ctx->devapc_irq = irq_of_parse_and_map(node, 0);
> > + if (!mtk_devapc_ctx->devapc_irq)
> > + return -EINVAL;
> > +
> > + /* CCF (Common Clock Framework) */
> > + mtk_devapc_ctx->devapc_infra_clk = devm_clk_get(&pdev->dev,
> > + "devapc-infra-clock");
> > +
> > + if (IS_ERR(mtk_devapc_ctx->devapc_infra_clk))
> > + return -EINVAL;
> > +
> > + proc_create("devapc_dbg", 0664, NULL, &devapc_dbg_fops);
> > +
> > + if (clk_prepare_enable(mtk_devapc_ctx->devapc_infra_clk))
> > + return -EINVAL;
> > +
> > + start_devapc();
> > +
> > + ret = devm_request_irq(&pdev->dev, mtk_devapc_ctx->devapc_irq,
> > + (irq_handler_t)devapc_violation_irq,
> > + IRQF_TRIGGER_NONE, "devapc", NULL);
> > + if (ret) {
> > + pr_err(PFX "request devapc irq failed, ret:%d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_devapc_probe);
>
> Why export probe function?
>
> > +
> > +int mtk_devapc_remove(struct platform_device *dev)
> > +{
> > + clk_disable_unprepare(mtk_devapc_ctx->devapc_infra_clk);
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_devapc_remove);
>
> Ditto.
>
> Regards,
> Chun-Kuang.
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Add MediaTek MT6873 devapc driver
2020-06-09 17:32 ` Add MediaTek MT6873 devapc driver Rob Herring
@ 2020-06-24 3:51 ` Neal Liu
0 siblings, 0 replies; 20+ messages in thread
From: Neal Liu @ 2020-06-24 3:51 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, wsd_upstream, linux-kernel, linux-mediatek,
Matthias Brugger, Neal Liu, linux-arm-kernel
On Tue, 2020-06-09 at 11:32 -0600, Rob Herring wrote:
> On Tue, Jun 09, 2020 at 06:24:19PM +0800, Neal Liu wrote:
> > These patch series introduce a MediaTek MT6873 devapc driver.
> >
> > MT6873 bus frabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violations are 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 devapc-mt6873 driver. The violation
> > information is printed in order to find the murderer.
>
> There's also a proposed driver with similar functionality[1]. Come up
> with a common solution.
>
> Rob
>
> [1] https://lore.kernel.org/linux-arm-kernel/20200128153806.7780-1-benjamin.gaignard@st.com/
Actually, Mediatek devapc HW do the similar things. But the real
"firewall" functionality is implemented in TrustZone instread of REE.
This devapc-mt6873 driver is mainly handled the violation.
Bus firewall framework seems not cover this parts.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-06-24 3:53 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-09 10:24 Add MediaTek MT6873 devapc driver Neal Liu
2020-06-09 10:24 ` [PATCH 1/2] dt-bindings: devapc: add bindings for devapc-mt6873 Neal Liu
2020-06-09 17:27 ` Rob Herring
[not found] ` <1591698261-22639-3-git-send-email-neal.liu@mediatek.com>
2020-06-09 16:01 ` [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver Chun-Kuang Hu
2020-06-11 9:26 ` Neal Liu
2020-06-11 11:01 ` Chun-Kuang Hu
2020-06-12 3:04 ` Neal Liu
2020-06-12 15:27 ` Chun-Kuang Hu
2020-06-15 2:12 ` Neal Liu
2020-06-12 23:20 ` Chun-Kuang Hu
2020-06-16 6:45 ` Neal Liu
2020-06-14 3:26 ` Chun-Kuang Hu
2020-06-15 2:43 ` Neal Liu
2020-06-15 14:14 ` Chun-Kuang Hu
2020-06-15 14:17 ` Chun-Kuang Hu
2020-06-16 6:09 ` Neal Liu
2020-06-15 15:51 ` Chun-Kuang Hu
2020-06-16 6:19 ` Neal Liu
2020-06-09 17:32 ` Add MediaTek MT6873 devapc driver Rob Herring
2020-06-24 3:51 ` Neal Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).