Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2020-10-08 at 10:45 +0200, Matthias Brugger wrote:
> 
> On 08/10/2020 04:35, Neal Liu wrote:
> > On Wed, 2020-10-07 at 12:44 +0200, Matthias Brugger wrote:
> >>
> >> On 27/08/2020 05:06, Neal Liu wrote:
> >>> MediaTek bus fabric provides TrustZone security support and data
> >>> protection to prevent slaves from being accessed by unexpected
> >>> masters.
> >>> The security violation is logged and sent to the processor for
> >>> further analysis or countermeasures.
> >>>
> >>> Any occurrence of security violation would raise an interrupt, and
> >>> it will be handled by mtk-devapc driver. The violation
> >>> information is printed in order to find the murderer.
> >>
> >> "The violation information is printed in order to find the responsible component."
> >>
> >> Nobody got actually killed, right :)
> > 
> > Correct !
> >>
> >>>
> >>> Signed-off-by: Neal Liu <neal.liu@xxxxxxxxxxxx>
> >>> ---
> >>>    drivers/soc/mediatek/Kconfig      |    9 ++
> >>>    drivers/soc/mediatek/Makefile     |    1 +
> >>>    drivers/soc/mediatek/mtk-devapc.c |  305 +++++++++++++++++++++++++++++++++++++
> >>>    3 files changed, 315 insertions(+)
> >>>    create mode 100644 drivers/soc/mediatek/mtk-devapc.c
> >>>
> >>> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> >>> index 59a56cd..1177c98 100644
> >>> --- a/drivers/soc/mediatek/Kconfig
> >>> +++ b/drivers/soc/mediatek/Kconfig
> >>> @@ -17,6 +17,15 @@ config MTK_CMDQ
> >>>    	  time limitation, such as updating display configuration during the
> >>>    	  vblank.
> >>>    
> >>> +config MTK_DEVAPC
> >>> +	tristate "Mediatek Device APC Support"
> >>> +	help
> >>> +	  Say yes here to enable support for Mediatek Device APC driver.
> >>> +	  This driver is mainly used to handle the violation which catches
> >>> +	  unexpected transaction.
> >>> +	  The violation information is logged for further analysis or
> >>> +	  countermeasures.
> >>> +
> >>>    config MTK_INFRACFG
> >>>    	bool "MediaTek INFRACFG Support"
> >>>    	select REGMAP
> >>> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> >>> index 01f9f87..abfd4ba 100644
> >>> --- a/drivers/soc/mediatek/Makefile
> >>> +++ b/drivers/soc/mediatek/Makefile
> >>> @@ -1,5 +1,6 @@
> >>>    # SPDX-License-Identifier: GPL-2.0-only
> >>>    obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> >>> +obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o
> >>>    obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
> >>>    obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> >>>    obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> >>> diff --git a/drivers/soc/mediatek/mtk-devapc.c b/drivers/soc/mediatek/mtk-devapc.c
> >>> new file mode 100644
> >>> index 0000000..0ba61d7
> >>> --- /dev/null
> >>> +++ b/drivers/soc/mediatek/mtk-devapc.c
> >>> @@ -0,0 +1,305 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * Copyright (C) 2020 MediaTek Inc.
> >>> + */
> >>> +
> >>> +#include <linux/clk.h>
> >>> +#include <linux/interrupt.h>
> >>> +#include <linux/iopoll.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/of_device.h>
> >>> +#include <linux/of_irq.h>
> >>> +#include <linux/of_address.h>
> >>> +
> >>> +#define VIO_MOD_TO_REG_IND(m)	((m) / 32)
> >>> +#define VIO_MOD_TO_REG_OFF(m)	((m) % 32)
> >>> +
> >>> +struct mtk_devapc_vio_dbgs {
> >>> +	union {
> >>> +		u32 vio_dbg0;
> >>> +		struct {
> >>> +			u32 mstid:16;
> >>> +			u32 dmnid:6;
> >>> +			u32 vio_w:1;
> >>> +			u32 vio_r:1;
> >>> +			u32 addr_h:4;
> >>> +			u32 resv:4;
> >>> +		} dbg0_bits;
> >>> +	};
> >>> +
> >>> +	u32 vio_dbg1;
> >>> +};
> >>> +
> >>> +struct mtk_devapc_data {
> >>> +	u32 vio_idx_num;
> >>> +	u32 vio_mask_offset;
> >>> +	u32 vio_sta_offset;
> >>> +	u32 vio_dbg0_offset;
> >>> +	u32 vio_dbg1_offset;
> >>> +	u32 apc_con_offset;
> >>> +	u32 vio_shift_sta_offset;
> >>> +	u32 vio_shift_sel_offset;
> >>> +	u32 vio_shift_con_offset;
> >>> +};
> >>
> >> Please describe the fields of the struct, that will make it easier to understand
> >> the driver.
> > 
> > Okay, I'll try to add more description about this struct. May be like:
> > 
> > struct mtk_devapc_data {
> > 	/* numbers of violation index */
> > 	u32 vio_idx_num;
> > 
> > 	/* reg offset */
> > 	u32 vio_mask_offset;
> > 	u32 vio_sta_offset;
> > 	u32 vio_dbg0_offset;
> > 	u32 vio_dbg1_offset;
> > 	u32 apc_con_offset;
> > 	u32 vio_shift_sta_offset;
> > 	u32 vio_shift_sel_offset;
> > 	u32 vio_shift_con_offset;
> > };
> > 
> >>
> >>> +
> >>> +struct mtk_devapc_context {
> >>> +	struct device *dev;
> >>> +	void __iomem *infra_base;
> >>> +	struct clk *infra_clk;
> >>> +	const struct mtk_devapc_data *data;
> >>> +};
> >>> +
> >>> +static void clear_vio_status(struct mtk_devapc_context *ctx)
> >>> +{
> >>> +	void __iomem *reg;
> >>> +	int i;
> >>> +
> >>> +	reg = ctx->infra_base + ctx->data->vio_sta_offset;
> >>> +
> >>> +	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
> >>> +		writel(GENMASK(31, 0), reg + 4 * i);
> >>> +
> >>> +	writel(GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1), 0),
> >>> +	       reg + 4 * i);
> >>> +}
> >>> +
> >>> +static void mask_module_irq(struct mtk_devapc_context *ctx, bool mask)
> >>> +{
> >>> +	void __iomem *reg;
> >>> +	u32 val;
> >>> +	int i;
> >>> +
> >>> +	reg = ctx->infra_base + ctx->data->vio_mask_offset;
> >>> +
> >>> +	if (mask)
> >>> +		val = GENMASK(31, 0);
> >>> +	else
> >>> +		val = 0;
> >>> +
> >>> +	for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1); i++)
> >>
> >> Do I get that right? We have a number of virtual IO identifier. Their
> >> correspondending interrupt are grouped in 32 bit registers. And we want to
> >> enable/disable them by writing 0 or 1. We have to take care of the last
> >> registers as it could be the case that vio_idx_num is not a multiple of 32, correct?
> >>
> >> In this case we should traverse VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1
> >> registers, which is (vio_idx_num / 32) - 1 and not (vio_idx_num - 1) / 32.
> >>
> > 
> > Yes, your understanding is correct. It should be
> > VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num) - 1 instead of
> > VIO_MOD_TO_REG_IND(ctx->data->vio_idx_num - 1).
> > 
> >>> +		writel(val, reg + 4 * i);
> >>> +
> >>> +	val = readl(reg + 4 * i);
> >>> +	if (mask)
> >>> +		val |= GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
> >>> +			       0);
> >>
> >> We have 511 IRQs, which gives us 31 bits in the last register to set/unset.
> >> Thats 510..0 bits, so from what I understand, once again we want
> >> GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num) - 1, 0)
> >> which is (vio_idx_num % 32) - 1
> >>
> >> Correct or do I understand something wrong?
> >> If so, same applies to clear_vio_status().
> >>
> > 
> > Correct. I'll fix it on next patch.
> > Thanks
> > 
> >>
> >>> +	else
> >>> +		val &= ~GENMASK(VIO_MOD_TO_REG_OFF(ctx->data->vio_idx_num - 1),
> >>> +				0);
> >>> +
> >>> +	writel(val, reg + 4 * i);
> >>> +}
> >>> +
> >>> +#define PHY_DEVAPC_TIMEOUT	0x10000
> >>> +
> >>> +/*
> >>> + * devapc_sync_vio_dbg - do "shift" mechansim" to get full violation information.
> >>> + *                       shift mechanism is depends on devapc hardware design.
> >>> + *                       Mediatek devapc set multiple slaves as a group.
> >>> + *                       When violation is triggered, violation info is kept
> >>> + *                       inside devapc hardware.
> >>> + *                       Driver should do shift mechansim to sync full violation
> >>> + *                       info to VIO_DBGs registers.
> >>> + *
> >>> + */
> >>> +static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
> >>> +{
> >>> +	void __iomem *pd_vio_shift_sta_reg;
> >>> +	void __iomem *pd_vio_shift_sel_reg;
> >>> +	void __iomem *pd_vio_shift_con_reg;
> >>> +	int min_shift_group;
> >>> +	int ret;
> >>> +	u32 val;
> >>> +
> >>> +	pd_vio_shift_sta_reg = ctx->infra_base +
> >>> +			       ctx->data->vio_shift_sta_offset;
> >>> +	pd_vio_shift_sel_reg = ctx->infra_base +
> >>> +			       ctx->data->vio_shift_sel_offset;
> >>> +	pd_vio_shift_con_reg = ctx->infra_base +
> >>> +			       ctx->data->vio_shift_con_offset;
> >>> +
> >>> +	/* Find the minimum shift group which has violation */
> >>> +	val = readl(pd_vio_shift_sta_reg);
> >>> +	if (!val)
> >>> +		return false;
> >>
> >> So bit 0 of selection register (pd_vio_shift_sel_reg) does not represent a
> >> violation group?
> >> I don't know how the HW works, but is seems odd to me. In case that's bit 0
> >> actually doesn't represent anything: how can an interrupt be triggered without
> >> any debug information present (means val == 0)?
> > 
> > This check implies HW status has something wrong. It cannot get any
> > debug information for this case.
> > It won't happen in normal scenario. Should we remove this check?
> > 
> 
> No I think the check is fine. I'd add a WARN() message as this is an indicator 
> that the HW is not working corretly.

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

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

I'll also add warn messages on next patch.

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

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






[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux