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

Matthias Brugger <matthias.bgg@xxxxxxxxx> · Mon, 13 Jul 2020 13:16:37 +0200

On 13/07/2020 09:45, Neal Liu wrote:
On Fri, 2020-07-10 at 14:14 +0200, Matthias Brugger wrote:

[snip]
+
+static int get_vio_slave_num(int slave_type)

I have a hard time to understand the usefullness of this, can you please explain.


The basic idea is to get total numbers of slaves. And we can use it to
scan all slaves which has been triggered violation.
I think I can pass it through DT data instead of using mtk_device_info
array. I'll send another patches to change it.

+{
+	if (slave_type == 0)
+		return ARRAY_SIZE(mtk_devices_infra);
+
+	return 0;
+}
+
+static u32 get_shift_group(struct mtk_devapc_context *devapc_ctx,
+			   int slave_type, int vio_idx)
+{
+	u32 vio_shift_sta;
+	void __iomem *reg;
+	int bit;
+
+	reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_SHIFT_STA, 0);
+	vio_shift_sta = readl(reg);
+
+	for (bit = 0; bit < 32; bit++) {
+		if ((vio_shift_sta >> bit) & 0x1) > +			break;
+	}
+
+	return bit;

We return the first position (from the right) of the rigster with the bit set to
one. Correct?
Can't we use __ffs() for this?

Yes, thanks for your reminds to use __ffs().
I'll revise it in next patches.


+}
+
+static int check_vio_mask_sta(struct mtk_devapc_context *devapc_ctx,
+			      int slave_type, u32 module, int pd_reg_type)
+{
+	u32 reg_index, reg_offset;
+	void __iomem *reg;
+	u32 value;
+
+	VIO_MASK_STA_REG_GET(module);
+
+	reg = mtk_devapc_pd_get(devapc_ctx, slave_type, pd_reg_type, reg_index);

reg = mtk_devapc_pd_get(devapc_ctx, slave_type, pd_reg_type,
VIO_MOD_TO_REG_IND(module));

Okay, I'll revise it in next patches.


+	value = readl(reg);
+
+	return ((value >> reg_offset) & 0x1);

return ((value >> VIO_MOD_TO_REG_OFF(module)) & 0x1);

Okay, I'll revise it in next patches.


+}
+
+static int check_vio_mask(struct mtk_devapc_context *devapc_ctx, int slave_type,
+			  u32 module)
+{
+	return check_vio_mask_sta(devapc_ctx, slave_type, module, VIO_MASK);
+}
+
+static int check_vio_status(struct mtk_devapc_context *devapc_ctx,
+			    int slave_type, u32 module)
+{
+	return check_vio_mask_sta(devapc_ctx, slave_type, module, VIO_STA);
+}
+
+static void clear_vio_status(struct mtk_devapc_context *devapc_ctx,
+			     int slave_type, u32 module)
+{
+	u32 reg_index, reg_offset;
+	void __iomem *reg;
+
+	VIO_MASK_STA_REG_GET(module);
+
+	reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_STA, reg_index);
+	writel(0x1 << reg_offset, reg);
+
+	if (check_vio_status(devapc_ctx, slave_type, module))
+		pr_err(PFX "%s: Clear failed, slave_type:0x%x, module_index:0x%x\n",
+		       __func__, slave_type, module);
+}
+
+static void mask_module_irq(struct mtk_devapc_context *devapc_ctx,
+			    int slave_type, u32 module, bool mask)
+{
+	u32 reg_index, reg_offset;
+	void __iomem *reg;
+	u32 value;
+
+	VIO_MASK_STA_REG_GET(module);
+
+	reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_MASK, reg_index);
+
+	value = readl(reg);
+	if (mask)
+		value |= (0x1 << reg_offset);
+	else
+		value &= ~(0x1 << reg_offset);
+
+	writel(value, reg);
+}
+
+#define TIMEOUT_MS		10000
+
+static int read_poll_timeout(void __iomem *addr, u32 mask)

That function is defined in include/linux/iopoll.h

+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(TIMEOUT_MS);
+
+	do {
+		if (readl_relaxed(addr) & mask)

Please use a variable where you write your value to and then check for the mask.
That maks the code easier to read and I think is part of the coding style.


Okay, I'll use the function in iopoll.h instead.
Thanks for your reminds.

+			return 0;
+
+	} while (!time_after(jiffies, timeout));
+
+	return (readl_relaxed(addr) & mask) ? 0 : -ETIMEDOUT;
+}
+
+/*
+ * sync_vio_dbg - start to get violation information by selecting violation
+ *		  group and enable violation shift.
+ *
+ * Returns sync done or not
+ */
+static u32 sync_vio_dbg(struct mtk_devapc_context *devapc_ctx, int slave_type,
+			u32 shift_bit)
+{
+	void __iomem *pd_vio_shift_sta_reg;
+	void __iomem *pd_vio_shift_sel_reg;
+	void __iomem *pd_vio_shift_con_reg;
+	u32 sync_done = 0;
+
+	pd_vio_shift_sta_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
+						 VIO_SHIFT_STA, 0);
+	pd_vio_shift_sel_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
+						 VIO_SHIFT_SEL, 0);
+	pd_vio_shift_con_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
+						 VIO_SHIFT_CON, 0);
+
+	writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
+	writel(0x1, pd_vio_shift_con_reg);
+
+	if (!read_poll_timeout(pd_vio_shift_con_reg, 0x2))
+		sync_done = 1;
+	else
+		pr_err(PFX "%s: Shift violation info failed\n", __func__);
+
+	/* Disable shift mechanism */

Please add a comment explaining what the shift mechanism is about.

Okay, I'll add a comment to explain it at the beginning of this
function.


+	writel(0x0, pd_vio_shift_con_reg);
+	writel(0x0, pd_vio_shift_sel_reg);
+	writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
+
+	return sync_done;
+}
+
+static void devapc_vio_info_print(struct mtk_devapc_context *devapc_ctx)
+{
+	struct mtk_devapc_vio_info *vio_info = devapc_ctx->vio_info;
+
+	/* Print violation information */
+	if (vio_info->write)
+		pr_info(PFX "Write Violation\n");
+	else if (vio_info->read)
+		pr_info(PFX "Read Violation\n");
+
+	pr_info(PFX "%s%x, %s%x, %s%x, %s%x\n",
+		"Vio Addr:0x", vio_info->vio_addr,
+		"High:0x", vio_info->vio_addr_high,
+		"Bus ID:0x", vio_info->master_id,
+		"Dom ID:0x", vio_info->domain_id);
+}
+
+static void devapc_extract_vio_dbg(struct mtk_devapc_context *devapc_ctx,
+				   int slave_type)
+{
+	void __iomem *vio_dbg0_reg, *vio_dbg1_reg;
+	struct mtk_devapc_vio_dbgs_desc *vio_dbgs;
+	struct mtk_devapc_vio_info *vio_info;
+	u32 dbg0;
+
+	vio_dbg0_reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_DBG0, 0);
+	vio_dbg1_reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_DBG1, 0);
+
+	vio_dbgs = devapc_ctx->vio_dbgs_desc;
+	vio_info = devapc_ctx->vio_info;
+
+	/* Extract violation information */
+	dbg0 = readl(vio_dbg0_reg);
+	vio_info->vio_addr = readl(vio_dbg1_reg);
+
+	vio_info->master_id = (dbg0 & vio_dbgs[MSTID].mask) >>
+			      vio_dbgs[MSTID].start_bit;
+	vio_info->domain_id = (dbg0 & vio_dbgs[DMNID].mask) >>
+			      vio_dbgs[DMNID].start_bit;
+	vio_info->write = ((dbg0 & vio_dbgs[VIO_W].mask) >>
+			   vio_dbgs[VIO_W].start_bit) == 1;
+	vio_info->read = ((dbg0 & vio_dbgs[VIO_R].mask) >>
+			  vio_dbgs[VIO_R].start_bit) == 1;
+	vio_info->vio_addr_high = (dbg0 & vio_dbgs[ADDR_H].mask) >>
+				  vio_dbgs[ADDR_H].start_bit;
+
+	devapc_vio_info_print(devapc_ctx);
+}
+
+/*
+ * mtk_devapc_dump_vio_dbg - shift & dump the violation debug information.
+ */
+static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *devapc_ctx,
+				    int slave_type, int *vio_idx)
+{
+	const struct mtk_device_info **device_info;
+	u32 shift_bit;
+	int i;
+
+	device_info = devapc_ctx->device_info;
+
+	for (i = 0; i < get_vio_slave_num(slave_type); i++) {
+		*vio_idx = device_info[slave_type][i].vio_index;
+
+		if (check_vio_mask(devapc_ctx, slave_type, *vio_idx))
+			continue;
+
+		if (!check_vio_status(devapc_ctx, slave_type, *vio_idx))
+			continue;
+
+		shift_bit = get_shift_group(devapc_ctx, slave_type, *vio_idx);
+
+		if (!sync_vio_dbg(devapc_ctx, slave_type, shift_bit))
+			continue;
+
+		devapc_extract_vio_dbg(devapc_ctx, slave_type);
+
+		return true;
+	}
+
+	return false;
+}
+
+/*
+ * 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 *devapc_ctx)
+{
+	const struct mtk_device_info **device_info;
+	int slave_type_num;
+	int vio_idx = -1;
+	int slave_type;
+
+	slave_type_num = devapc_ctx->slave_type_num;
+	device_info = devapc_ctx->device_info;
+
+	for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
+		if (!mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type, &vio_idx))
+			continue;
+
+		/* Ensure that violation info are written before
+		 * further operations
+		 */
+		smp_mb();
+
+		mask_module_irq(devapc_ctx, slave_type, vio_idx, true);
+
+		clear_vio_status(devapc_ctx, slave_type, vio_idx);
+
+		mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * start_devapc - initialize devapc status and start receiving interrupt
+ *		  while devapc violation is triggered.
+ */
+static void start_devapc(struct mtk_devapc_context *devapc_ctx)
+{
+	const struct mtk_device_info **device_info;
+	void __iomem *pd_vio_shift_sta_reg;
+	void __iomem *pd_apc_con_reg;
+	u32 vio_shift_sta;
+	int slave_type, slave_type_num;
+	int i, vio_idx;
+
+	device_info = devapc_ctx->device_info;
+	slave_type_num = devapc_ctx->slave_type_num;
+
+	for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
+		pd_apc_con_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
+						   APC_CON, 0);
+		pd_vio_shift_sta_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
+							 VIO_SHIFT_STA, 0);
+		if (!pd_apc_con_reg || !pd_vio_shift_sta_reg)
+			return;
+
+		/* Clear devapc violation status */
+		writel(BIT(31), pd_apc_con_reg);
+
+		/* Clear violation shift status */
+		vio_shift_sta = readl(pd_vio_shift_sta_reg);
+		if (vio_shift_sta)
+			writel(vio_shift_sta, pd_vio_shift_sta_reg);
+
+		/* Clear slave violation status */
+		for (i = 0; i < get_vio_slave_num(slave_type); i++) {
+			vio_idx = device_info[slave_type][i].vio_index;
+
+			clear_vio_status(devapc_ctx, slave_type, vio_idx);
+
+			mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
+		}
+	}
+}
+
+static int mtk_devapc_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct mtk_devapc_context *devapc_ctx;
+	struct clk *devapc_infra_clk;
+	u32 vio_dbgs_num, pds_num;
+	u8 slave_type_num;
+	u32 devapc_irq;
+	size_t size;
+	int i, ret;
+
+	if (IS_ERR(node))
+		return -ENODEV;
+
+	devapc_ctx = devm_kzalloc(&pdev->dev, sizeof(struct mtk_devapc_context),
+				  GFP_KERNEL);
+	if (!devapc_ctx)
+		return -ENOMEM;
+
+	if (of_property_read_u8(node, "mediatek-slv_type_num", &slave_type_num))
+		return -ENXIO;
+
+	devapc_ctx->slave_type_num = slave_type_num;
+
+	size = slave_type_num * sizeof(void *);
+	devapc_ctx->devapc_pd_base = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	if (!devapc_ctx->devapc_pd_base)
+		return -ENOMEM;
+
+	size = slave_type_num * sizeof(struct mtk_device_info *);
+	devapc_ctx->device_info = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	if (!devapc_ctx->device_info)
+		return -ENOMEM;
+
+	for (i = 0; i < slave_type_num; i++) {
+		devapc_ctx->devapc_pd_base[i] = of_iomap(node, i);
+		if (!devapc_ctx->devapc_pd_base[i])
+			return -EINVAL;
+
+		if (i == 0)
+			devapc_ctx->device_info[i] = mtk_devices_infra;
+	}
+
+	size = sizeof(struct mtk_devapc_vio_info);
+	devapc_ctx->vio_info = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	if (!devapc_ctx->vio_info)
+		return -ENOMEM;
+
+	vio_dbgs_num = of_property_count_u32_elems(node, "mediatek-vio_dbgs");
+	if (vio_dbgs_num <= 0)
+		return -ENXIO;
+
+	size = (vio_dbgs_num / 2) * sizeof(struct mtk_devapc_vio_dbgs_desc);
+	devapc_ctx->vio_dbgs_desc = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	if (!devapc_ctx->vio_dbgs_desc)
+		return -ENOMEM;
+
+	for (i = 0; i < vio_dbgs_num / 2; i++) {
+		if (of_property_read_u32_index(node, "mediatek-vio_dbgs",
+					       i * 2,
+					       &devapc_ctx->vio_dbgs_desc[i].mask))
+			return -ENXIO;
+
+		if (of_property_read_u32_index(node, "mediatek-vio_dbgs",
+					       (i * 2) + 1,
+					       &devapc_ctx->vio_dbgs_desc[i].start_bit))
+			return -ENXIO;
+	}
+
+	pds_num = of_property_count_u32_elems(node, "mediatek-pds_offset");
+	if (pds_num <= 0)
+		return -ENXIO;
+
+	size = pds_num * sizeof(u32);
+	devapc_ctx->pds_offset = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
+	if (!devapc_ctx->pds_offset)
+		return -ENOMEM;
+
+	for (i = 0; i < pds_num; i++) {
+		if (of_property_read_u32_index(node, "mediatek-pds_offset", i,
+					       &devapc_ctx->pds_offset[i]))
+			return -ENXIO;
+	}
+
+	devapc_irq = irq_of_parse_and_map(node, 0);
+	if (!devapc_irq)
+		return -EINVAL;
+
+	devapc_infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
+	if (IS_ERR(devapc_infra_clk))
+		return -EINVAL;
+
+	if (clk_prepare_enable(devapc_infra_clk))
+		return -EINVAL;
+
+	start_devapc(devapc_ctx);
+
+	ret = devm_request_irq(&pdev->dev, devapc_irq,
+			       (irq_handler_t)devapc_violation_irq,
+			       IRQF_TRIGGER_NONE, "devapc", devapc_ctx);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mtk_devapc_remove(struct platform_device *dev)
+{
+	return 0;
+}
+
+static const struct of_device_id mtk_devapc_dt_match[] = {
+	{ .compatible = "mediatek,mt6779-devapc" },
+	{},
+};
+
+static struct platform_driver mtk_devapc_driver = {
+	.probe = mtk_devapc_probe,
+	.remove = mtk_devapc_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = mtk_devapc_dt_match,
+	},
+};
+
+module_platform_driver(mtk_devapc_driver);
+
+MODULE_DESCRIPTION("Mediatek Device APC Driver");
+MODULE_AUTHOR("Neal Liu <neal.liu@xxxxxxxxxxxx>");
+MODULE_LICENSE("GPL");

diff --git a/drivers/soc/mediatek/mtk-devapc.h b/drivers/soc/mediatek/mtk-devapc.h
new file mode 100644
index 0000000..ab2cb14
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-devapc.h
@@ -0,0 +1,670 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020 MediaTek Inc.
+ */
+
+#ifndef __MTK_DEVAPC_H__
+#define __MTK_DEVAPC_H__
+
+#define PFX			"[DEVAPC]: "

use dev_err() and friends instead.

Okay, I'll remove it.


+
+#define VIO_MASK_STA_REG_GET(m) \
+({ \
+	typeof(m) (_m) = (m); \
+	reg_index = _m / 32; \
+	reg_offset = _m % 32; \
+})

don't do that. no explicit variable assingment in a macro, the macro should
return the value.

Okay, I'll revise it in next patches.


+
+enum DEVAPC_PD_REG_TYPE {
+	VIO_MASK = 0,
+	VIO_STA,
+	VIO_DBG0,
+	VIO_DBG1,
+	APC_CON,
+	VIO_SHIFT_STA,
+	VIO_SHIFT_SEL,
+	VIO_SHIFT_CON,
+	PD_REG_TYPE_NUM,
+};
+
+enum DEVAPC_VIO_DBGS_TYPE {
+	MSTID = 0,
+	DMNID,
+	VIO_W,
+	VIO_R,
+	ADDR_H,
+};
+
+struct mtk_device_info {
+	int sys_index;
+	int ctrl_index;
+	int vio_index;
+};
+
+static struct mtk_device_info mtk_devices_infra[] = {

That's for mt6779, correct? Should be stated in the name.

Okay. I have another way to reach the goal without using this struct
array. I'll send another patches.


[...]

+
+struct mtk_devapc_vio_info {
+	bool read;
+	bool write;
+	u32 vio_addr;
+	u32 vio_addr_high;
+	u32 master_id;
+	u32 domain_id;
+};
+
+struct mtk_devapc_vio_dbgs_desc {
+	u32 mask;
+	u32 start_bit;
+};
+
+struct mtk_devapc_context {
+	u8 slave_type_num;
+	void __iomem **devapc_pd_base;
+	const struct mtk_device_info **device_info;
+	struct mtk_devapc_vio_info *vio_info;
+	struct mtk_devapc_vio_dbgs_desc *vio_dbgs_desc;
+	u32 *pds_offset;
+};
+

Not sure if I get this right:

struct mtk_devapc_offset {
	u32 vio_mask;
	u32 vio_sta;
	u32 vio_dbg0;
	u32 vio_dbg1;
	...
}

struct mtk_devapc_context {
	u8 pd_base_num;
	void __iomem **devapc_pd_base;
	struct mtk_devapc_offset *offset;
	const struct mtk_device_info **device_info;
	struct mtk_devapc_vio_info *vio_info;
	struct mtk_devapc_vio_dbgs_desc *vio_dbgs_desc;
};

With this I think we can get rid of mtk_devapc_pd_get().


mtk_devapc_pd_get() is used to calculate the vaddr of devapc pd
register. It's based on different slave_type, pd_reg_type and reg_idx.
I don't think it can be replaced with such simple data structures.


How I understand the code:
Every slave_type has a base memory represented by the **devapc_pd_base array.
Inside each base memory chunk you have an offset depending on the pd_reg_type, 
but the offset is the same for all base memory chunks. This offset is 
represented by struct mtk_devapc_offset.
If pd_reg_type is VIO_MASK or VIO_STA we have to further read the value based on 
an index represented by reg_idx. So if we  add 0x4 for each reg_idx. So we have 
for example for:
int check_vio_mask(struct mtk_devapc_context *ctx, inst slave_type, u32 module)
{
	reg = ctx->devapc_pd_base[slave_type] + ctx->offset.vio_mask;
	reg += 0x4 * VIO_MOD_TO_REG_IND(module);

	value = readl(reg);
	return ((value >> VIO_TO_REG_OFF(module)) & 0x1);
}

similarly:
u32 get_shift_group(struct mtk_devapc_context *ctx, int slave_type, int vio_idx)
{
	reg = ctx->devapc_pd_base[slave_type] + ctx->offset.vio_shift_sta;

	value = readl(reg);
	bit = __ffs(...);
}

What does us buy that? When looking on the function we understand how the 
register layout in HW looks like. We have a base value with an offset and in 
case of VIO_MASK and VIO_STA we have to shift the value.

By the way, right now in mtk_devapc_pd_get you are doing pointer arithmetic with 
a void pointer. That's not a good approach, please define the pointer to point 
to the value you want to read. I understand that's a 32 bit register.

Regards
Matthias


Sorry I'm not able to review the whole driver right now. Please also have a look
on my comments from v1.

We will have to go little by little to get this into a good state. In case it
makes sense to have this in the kernel at all.

Regards,
Matthias

I'm appreciated for your review. It helps me to write better code and
get closer to the kernel.








