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

[Neal Liu <neal.liu@mediatek.com>]

On Mon, 2020-07-13 at 13:16 +0200, Matthias Brugger wrote:
> 
> 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@mediatek.com>");
> >>> +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.
> 

Yes, you're right. We can do calculation in each place instead of
calling mtk_devapc_pd_get(). Is is better to do this instead of function
call?
Sorry for my misunderstanding about previous comments.

> 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
> 

I'm not quite understand this comment. Does below arithmetic not meet
your statement? Could you explain more details?

reg = devapc_ctx->devapc_pd_base[slave_type] +
	devapc_ctx->pds_offset[pd_reg_type];

> > 
> >> 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.
> > 
> >